WEB-657: Working Capital product with Advanced accounting#3540
Open
alberto-art3ch wants to merge 1 commit intoopenMF:devfrom
Open
WEB-657: Working Capital product with Advanced accounting#3540alberto-art3ch wants to merge 1 commit intoopenMF:devfrom
alberto-art3ch wants to merge 1 commit intoopenMF:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
New Advanced Accounting Component src/app/products/loan-products/common/loan-product-summary/.../loan-product-summary-adv-accounting.component.html, ... .component.scss, ... .component.ts |
Adds LoanProductSummaryAdvAccountingComponent (template, styles, inputs) to render conditional advanced-accounting tables and accept all mapping arrays and displayed-column configs. |
Loan Product Summary src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html, ... .component.ts |
Replaces inline advanced-accounting tables with the new component; makes currency access null-safe; consolidates mapping initialization and accepts both ID and object forms. |
Accounting Step UI & Form src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/... .component.html, ... .component.ts |
Replaces many inline table editors with reusable mapping-rule components; refactors FormArray item shape to { value, glAccount }; patches advancedAccountingRules for CASH_BASED and adjusts accounting-rule control flow. |
Advanced Accounting Mapping Rule .../advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts, ... .component.html |
Makes dialog action-aware (add/edit), calls fillCurrentFormValues() on init, updates add/edit handling to use response.data.value.*, skips reason filtering during edit, and updates GL Account header label. |
Models & Minor Cleanup src/app/products/loan-products/models/loan-product.model.ts, .../near-breach-configuration/... .component.ts |
Changes AccountingMappingDTO.glAccount type to `GLAccount |
Translations src/assets/translations/*.json (en-US, cs-CS, de-DE, es-CL, es-MX, fr-FR, it-IT, ko-KO, lt-LT, lv-LV, ne-NE, pt-PT, sw-SW) |
Adds "Fund Sources" key across locales and renames "Charge-off reason" → "Charge-off Reason" in translation files. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
- WEB-657: Working Capital product with GL Account mappings #3520 — Modifies loan-product accounting/advanced-accounting UI and mapping data flow; overlaps summary/template refactor.
- WEB-342: Advance Accounting rule can not be edited for classification type #2702 — Changes add/edit handling and table-update logic for advanced-accounting-mapping-rule; overlaps dialog/add-edit shaping.
- WEB-316: Advance Accounting for loan transaction classification #2701 — Adjusts GL account handling/typing for advanced-accounting mappings; relates to glAccount nullability.
Suggested reviewers
- adamsaghy
- IOhacker
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main change: adding advanced accounting rules support for Working Capital loan products, which is consistent with the primary objective and file changes across the codebase. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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.
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.ts (1)
252-289:⚠️ Potential issue | 🟠 MajorLoan-product mapping initialization changed shape and breaks existing id-based table/edit flow.
In the loan-product branch, these arrays are now initialized as
{ value, glAccount }, but the existing loan-product UI/edit logic still expects id fields (paymentTypeId,chargeId,chargeOffReasonCodeValueId, etc.). This causes incorrect rendering and edit defaults for preloaded mappings.🔧 Suggested fix (keep loan-product branch id-based)
this.loanProductAccountingForm.setControl( 'paymentChannelToFundSourceMappings', this.formBuilder.array( (this.loanProductsTemplate.paymentChannelToFundSourceMappings || []).map((paymentFundSource: any) => ({ - value: paymentFundSource.paymentType, - glAccount: paymentFundSource.fundSourceAccount + paymentTypeId: paymentFundSource.paymentType?.id ?? paymentFundSource.paymentTypeId, + fundSourceAccountId: paymentFundSource.fundSourceAccount?.id ?? paymentFundSource.fundSourceAccountId })) ) ); this.loanProductAccountingForm.setControl( 'feeToIncomeAccountMappings', this.formBuilder.array( (this.loanProductsTemplate.feeToIncomeAccountMappings || []).map((feesIncome: any) => ({ - value: feesIncome.charge, - glAccount: feesIncome.incomeAccount + chargeId: feesIncome.charge?.id ?? feesIncome.chargeId, + incomeAccountId: feesIncome.incomeAccount?.id ?? feesIncome.incomeAccountId })) ) ); this.loanProductAccountingForm.setControl( 'penaltyToIncomeAccountMappings', this.formBuilder.array( (this.loanProductsTemplate.penaltyToIncomeAccountMappings || []).map((penaltyIncome: any) => ({ - value: penaltyIncome.charge, - glAccount: penaltyIncome.incomeAccount + chargeId: penaltyIncome.charge?.id ?? penaltyIncome.chargeId, + incomeAccountId: penaltyIncome.incomeAccount?.id ?? penaltyIncome.incomeAccountId })) ) ); this.loanProductAccountingForm.setControl( 'chargeOffReasonToExpenseAccountMappings', this.formBuilder.array( (this.loanProductsTemplate.chargeOffReasonToExpenseAccountMappings || []).map( (m: ChargeOffReasonToExpenseAccountMapping) => ({ - value: m.reasonCodeValue, - glAccount: m.expenseAccount + chargeOffReasonCodeValueId: m.reasonCodeValue?.id ?? m.chargeOffReasonCodeValueId, + expenseAccountId: m.expenseAccount?.id ?? m.expenseAccountId }) ) ) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.ts` around lines 252 - 289, The mapping arrays are initialized with {value, glAccount} but the UI/edit flow expects id fields (e.g., paymentTypeId, chargeId, chargeOffReasonCodeValueId); update the setControl mappings in loan-product-accounting-step.component.ts so paymentChannelToFundSourceMappings maps to { paymentTypeId: paymentFundSource.paymentType, glAccount: paymentFundSource.fundSourceAccount }, feeToIncomeAccountMappings and penaltyToIncomeAccountMappings map to { chargeId: feesIncome.charge, glAccount: feesIncome.incomeAccount } / { chargeId: penaltyIncome.charge, glAccount: penaltyIncome.incomeAccount }, and chargeOffReasonToExpenseAccountMappings maps to { chargeOffReasonCodeValueId: m.reasonCodeValue, glAccount: m.expenseAccount } so the form shape matches the existing id-based UI/edit logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.html`:
- Around line 71-78: The template directly dereferences mapped objects (e.g.,
penaltyIncome.charge.name, penaltyIncome.incomeAccount.name and similar
expressions), which can crash if the lookup is missing; update these bindings in
loan-product-summary-adv-accounting template to use the safe navigation operator
and a simple fallback, for example change penaltyIncome.charge.name to
penaltyIncome.charge?.name || '' (and penaltyIncome.incomeAccount.name to
penaltyIncome.incomeAccount?.name || ''), and do the same for all other
occurrences such as any *.glCode bindings (e.g., incomeAccount?.glCode || '')
mentioned in the review.
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.ts`:
- Around line 53-55: Remove the debug console.log from the component
constructor: delete the line logging paymentChannelToFundSourceMappings inside
the constructor of the LoanProductSummaryAdvAccountingComponent; if runtime
diagnostics are required instead use a proper injectable logger or conditional
logging behind an environment flag (or move one-time checks to ngOnInit) rather
than console.log.
- Around line 45-51: The `@Input` properties in
LoanProductSummaryAdvAccountingComponent use unsafe any types; replace each any
| null with the explicit types from src/app/shared/models/general.model.ts:
change paymentChannelToFundSourceMappings to PaymentChannelToFundSourceMapping[]
| null, feeToIncomeAccountMappings and penaltyToIncomeAccountMappings to
ChargeToIncomeAccountMapping[] | null, chargeOffReasonToExpenseAccountMappings
and writeOffReasonsToExpenseMappings to ChargeOffReasonToExpenseAccountMapping[]
| null, and buydownFeeClassificationToIncomeAccountMappings and
capitalizedIncomeClassificationToIncomeAccountMappings to
ClassificationToIncomeAccountMapping[] | null; update the import list in
loan-product-summary-adv-accounting.component.ts to import these interfaces and
ensure the `@Input`(...) declarations (paymentChannelToFundSourceMappings,
feeToIncomeAccountMappings, penaltyToIncomeAccountMappings,
chargeOffReasonToExpenseAccountMappings,
buydownFeeClassificationToIncomeAccountMappings,
capitalizedIncomeClassificationToIncomeAccountMappings,
writeOffReasonsToExpenseMappings) use the precise types.
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts`:
- Around line 285-293: In the loop that builds
this.chargeOffReasonToExpenseAccountMappings, optionData returned by
optionDataLookUp can be null so avoid dereferencing
optionData.id/optionData.value; modify the code in the
loanProduct.chargeOffReasonToExpenseAccountMappings processing to first check if
optionData is truthy (from optionDataLookUp) and either skip adding the mapping
or supply a safe fallback CodeValue (e.g., { id: null, name: '' }) before
pushing; keep the expenseAccount via glAccountLookUp(m.expenseAccountId,
expenseAccountData) as-is and ensure the pushed object still conforms to the
ChargeOffReasonToExpenseAccountMapping/CodeValue types.
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.html`:
- Around line 703-755: Replace hardcoded user-facing strings passed to the
mifosx-advanced-accounting-mapping-rule inputs (textField and textHeading) with
translation keys using the ngx-translate pipe or translate service (e.g., use
"('SOME.KEY' | translate)" or translate.instant in the component). Update each
instance: textField values 'Fund Sources', 'Fees', 'Penalty', 'Charge-off
Reason', 'WriteOff Reason' and corresponding textHeading values to use
appropriate i18n keys (create descriptive keys like
loanProduct.accounting.fundSources and
loanProduct.accounting.fundSources.heading), and then run npm run
translations:extract to add the new keys to the translation files.
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.ts`:
- Line 637: The valueChanges subscription for the 'advancedAccountingRules'
toggle currently has empty branches so toggling does nothing; implement the
branches so when the control becomes true you create and populate the related
mapping controls (e.g. add the FormGroup/FormArray for advanced accounting
mappings to this.loanProductAccountingForm and initialize any default mapping
entries), and when it becomes false you remove or reset those mapping controls
(call this.loanProductAccountingForm.removeControl('advancedAccountingRules') if
appropriate and clear/remove any related FormArray entries to avoid stale
mappings). Add small helper methods like addAdvancedAccountingRuleControls() and
removeAdvancedAccountingRuleControls() and call them from the subscription and
any existing places where mappings are created/cleared (also fix the same
empty-branch logic referenced around the 699-707 block).
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.ts`:
- Around line 101-105: The getter loanProductCurrency currently returns form
values directly from loanProductCurrencyForm so optional numeric controls like
inMultiplesOf and installmentAmountInMultiplesOf remain as '' when empty; update
the loanProductCurrency getter to normalize those fields before returning (e.g.,
check result.inMultiplesOf and result.installmentAmountInMultiplesOf and if they
are empty strings convert them to null or delete those keys) so the payload does
not contain empty string values.
In `@src/assets/translations/ko-KO.json`:
- Line 2090: The translation for the key "Fund Sources" uses the awkward plural
form "자금 출처들"; update the value for the "Fund Sources" key to the more natural
and consistent "자금 출처" (replace the current "자금 출처들" with "자금 출처" in the JSON).
---
Outside diff comments:
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.ts`:
- Around line 252-289: The mapping arrays are initialized with {value,
glAccount} but the UI/edit flow expects id fields (e.g., paymentTypeId,
chargeId, chargeOffReasonCodeValueId); update the setControl mappings in
loan-product-accounting-step.component.ts so paymentChannelToFundSourceMappings
maps to { paymentTypeId: paymentFundSource.paymentType, glAccount:
paymentFundSource.fundSourceAccount }, feeToIncomeAccountMappings and
penaltyToIncomeAccountMappings map to { chargeId: feesIncome.charge, glAccount:
feesIncome.incomeAccount } / { chargeId: penaltyIncome.charge, glAccount:
penaltyIncome.incomeAccount }, and chargeOffReasonToExpenseAccountMappings maps
to { chargeOffReasonCodeValueId: m.reasonCodeValue, glAccount: m.expenseAccount
} so the form shape matches the existing id-based UI/edit logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17a07bd4-6c5e-4d92-9f24-96c4509aa627
📒 Files selected for processing (23)
src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.htmlsrc/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.scsssrc/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.tssrc/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.htmlsrc/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
Comment on lines
+71
to
+78
| {{ penaltyIncome.charge.name }} | ||
| </td> | ||
| </ng-container> | ||
| <ng-container matColumnDef="incomeAccountId"> | ||
| <th mat-header-cell *matHeaderCellDef>{{ 'labels.inputs.Income Account' | translate }}</th> | ||
| <td mat-cell *matCellDef="let penaltyIncome"> | ||
| {{ penaltyIncome.incomeAccount.name }} | ||
| </td> |
There was a problem hiding this comment.
Guard mapped entities with safe navigation to avoid template crashes.
These cells assume lookup objects always exist. If a mapped option/account is missing, direct dereference (.name, .glCode) can break rendering.
🛡️ Suggested fix
- {{ penaltyIncome.incomeAccount.name }}
+ {{ penaltyIncome.incomeAccount?.name || '-' }}
- ({{ chargeOffReasonToExpenseAccountMapping.expenseAccount.glCode }})
- {{ chargeOffReasonToExpenseAccountMapping.expenseAccount.name }}
+ ({{ chargeOffReasonToExpenseAccountMapping.expenseAccount?.glCode || '-' }})
+ {{ chargeOffReasonToExpenseAccountMapping.expenseAccount?.name || '-' }}
- {{ classificationToIncomeAccountMapping.classificationCodeValue.name }}
+ {{ classificationToIncomeAccountMapping.classificationCodeValue?.name || '-' }}
- ({{ classificationToIncomeAccountMapping.incomeAccount.glCode }})
- {{ classificationToIncomeAccountMapping.incomeAccount.name }}
+ ({{ classificationToIncomeAccountMapping.incomeAccount?.glCode || '-' }})
+ {{ classificationToIncomeAccountMapping.incomeAccount?.name || '-' }}
- {{ capitalizedIncomeClassificationToIncomeAccountMapping.classificationCodeValue.name }}
+ {{ capitalizedIncomeClassificationToIncomeAccountMapping.classificationCodeValue?.name || '-' }}
- ({{ capitalizedIncomeClassificationToIncomeAccountMapping.incomeAccount.glCode }})
- {{ capitalizedIncomeClassificationToIncomeAccountMapping.incomeAccount.name }}
+ ({{ capitalizedIncomeClassificationToIncomeAccountMapping.incomeAccount?.glCode || '-' }})
+ {{ capitalizedIncomeClassificationToIncomeAccountMapping.incomeAccount?.name || '-' }}
- {{ writeOffReasonsToExpenseMapping.reasonCodeValue.name }}
+ {{ writeOffReasonsToExpenseMapping.reasonCodeValue?.name || '-' }}
- ({{ writeOffReasonsToExpenseMapping.expenseAccount.glCode }})
- {{ writeOffReasonsToExpenseMapping.expenseAccount.name }}
+ ({{ writeOffReasonsToExpenseMapping.expenseAccount?.glCode || '-' }})
+ {{ writeOffReasonsToExpenseMapping.expenseAccount?.name || '-' }}Also applies to: 94-102, 118-126, 146-154, 170-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.html`
around lines 71 - 78, The template directly dereferences mapped objects (e.g.,
penaltyIncome.charge.name, penaltyIncome.incomeAccount.name and similar
expressions), which can crash if the lookup is missing; update these bindings in
loan-product-summary-adv-accounting template to use the safe navigation operator
and a simple fallback, for example change penaltyIncome.charge.name to
penaltyIncome.charge?.name || '' (and penaltyIncome.incomeAccount.name to
penaltyIncome.incomeAccount?.name || ''), and do the same for all other
occurrences such as any *.glCode bindings (e.g., incomeAccount?.glCode || '')
mentioned in the review.
| this.loanProductAccountingForm.removeControl('deferredIncomeLiabilityAccountId'); | ||
|
|
||
| // this.loanProductAccountingForm.removeControl('advancedAccountingRules'); | ||
| this.loanProductAccountingForm.removeControl('advancedAccountingRules'); |
There was a problem hiding this comment.
advancedAccountingRules toggle is wired but no-op in working-capital flow.
The new valueChanges subscription has empty branches, so toggling the checkbox does not add/reset mapping controls. This can leave stale mappings active when rules are unchecked.
♻️ Suggested fix
this.loanProductAccountingForm
.get('advancedAccountingRules')
.valueChanges.subscribe((advancedAccountingRules: boolean) => {
if (advancedAccountingRules) {
+ this.loanProductAccountingForm.setControl('paymentChannelToFundSourceMappings', this.formBuilder.array([]));
+ this.loanProductAccountingForm.setControl('feeToIncomeAccountMappings', this.formBuilder.array([]));
+ this.loanProductAccountingForm.setControl('penaltyToIncomeAccountMappings', this.formBuilder.array([]));
+ this.loanProductAccountingForm.setControl('chargeOffReasonToExpenseAccountMappings', this.formBuilder.array([]));
+ this.loanProductAccountingForm.setControl('writeOffReasonsToExpenseMappings', this.formBuilder.array([]));
} else {
+ this.loanProductAccountingForm.setControl('paymentChannelToFundSourceMappings', this.formBuilder.array([]));
+ this.loanProductAccountingForm.setControl('feeToIncomeAccountMappings', this.formBuilder.array([]));
+ this.loanProductAccountingForm.setControl('penaltyToIncomeAccountMappings', this.formBuilder.array([]));
+ this.loanProductAccountingForm.setControl('chargeOffReasonToExpenseAccountMappings', this.formBuilder.array([]));
+ this.loanProductAccountingForm.setControl('writeOffReasonsToExpenseMappings', this.formBuilder.array([]));
}
});Also applies to: 699-707
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.ts`
at line 637, The valueChanges subscription for the 'advancedAccountingRules'
toggle currently has empty branches so toggling does nothing; implement the
branches so when the control becomes true you create and populate the related
mapping controls (e.g. add the FormGroup/FormArray for advanced accounting
mappings to this.loanProductAccountingForm and initialize any default mapping
entries), and when it becomes false you remove or reset those mapping controls
(call this.loanProductAccountingForm.removeControl('advancedAccountingRules') if
appropriate and clear/remove any related FormArray entries to avoid stale
mappings). Add small helper methods like addAdvancedAccountingRuleControls() and
removeAdvancedAccountingRuleControls() and call them from the subscription and
any existing places where mappings are created/cleared (also fix the same
empty-branch logic referenced around the 699-707 block).
alberto-art3ch
force-pushed
the
WEB-657/working-capital-loan-product-advanced-accounting
branch
from
c3f5349 to
f6597e0
Compare
alberto-art3ch
force-pushed
the
WEB-657/working-capital-loan-product-advanced-accounting
branch
from
f6597e0 to
0b90e2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html (1)
60-64:⚠️ Potential issue | 🟡 MinorInconsistent null safety for currency access.
Line 60 now uses optional chaining (
loanProduct.currency?.code), but line 64 directly accessesloanProduct.currency.decimalPlaceswithout optional chaining. Ifcurrencyis undefined anddigitsAfterDecimalis also nullish, this will throw a runtime error.🔧 Suggested fix
- <span class="flex-60">{{ loanProduct.digitsAfterDecimal ?? loanProduct.currency.decimalPlaces }}</span> + <span class="flex-60">{{ loanProduct.digitsAfterDecimal ?? loanProduct.currency?.decimalPlaces }}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html` around lines 60 - 64, The template uses optional chaining for loanProduct.currency?.code but directly accesses loanProduct.currency.decimalPlaces which can throw if currency is undefined; update the binding that shows decimal places (the expression using loanProduct.digitsAfterDecimal and loanProduct.currency.decimalPlaces) to guard access to currency (e.g., use optional chaining or a safe fallback) so that if loanProduct.currency is undefined and digitsAfterDecimal is nullish it doesn't raise a runtime error while preserving the existing precedence of loanProduct.digitsAfterDecimal over currency.decimalPlaces.src/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.ts (1)
76-94:⚠️ Potential issue | 🟡 MinorAdd missing
Validators.min(1)to form controls that expect min error handling in template.The form controls
inMultiplesOfandinstallmentAmountInMultiplesOfare missing validators but the template expects to display min validation errors. Both fields havemin="1"in the HTML and show error messages when the min validator fails, but the form controls are created without these validators, creating a mismatch between form validation and template error handling.🔧 Suggested fix: add min validators to match template expectations
inMultiplesOf: [''] + inMultiplesOf: ['', Validators.min(1)] }); if (this.loanProductService.isLoanProduct) { - this.loanProductCurrencyForm.addControl('installmentAmountInMultiplesOf', new UntypedFormControl('')); + this.loanProductCurrencyForm.addControl('installmentAmountInMultiplesOf', new UntypedFormControl('', Validators.min(1))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.ts` around lines 76 - 94, In createLoanProductCurrencyForm add Validators.min(1) to the FormControls that the template expects min validation for: update the 'inMultiplesOf' control to include Validators.min(1), and when adding 'installmentAmountInMultiplesOf' inside the isLoanProduct branch add the same Validators.required? or at least Validators.min(1) (and Validators.required if the template expects required) so the form validation matches the template error handling; reference createLoanProductCurrencyForm, controls 'inMultiplesOf' and 'installmentAmountInMultiplesOf'.
🧹 Nitpick comments (1)
src/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.ts (1)
32-32: Remove unuseddestroyRefinjection.
DestroyRefis injected but never used in this component, likely leftover from removed conditional validation logic.♻️ Suggested fix
- private destroyRef = inject(DestroyRef);Also remove
DestroyReffrom the import statement on line 9.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.ts` at line 32, Remove the unused DestroyRef injection: delete the private destroyRef = inject(DestroyRef); field (symbol: destroyRef) from the component class and also remove DestroyRef from the import list so there are no unused imports (symbol: DestroyRef, component: loan-product-currency-step.component). Ensure no remaining references to destroyRef exist after removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts`:
- Around line 153-154: The view-mode bug is caused by a casing mismatch: change
the assignment that sets this.buydownFeeClassificationToIncomeAccountMappings to
read the API property with a lowercase 'f'
(buydownfeeClassificationToIncomeAccountMappings) so it matches the rest of the
file; update any other references in LoanProductSummaryComponent that rely on
this.loanProduct property to use buydownfeeClassificationToIncomeAccountMappings
consistently (or normalize the value once when loading the loanProduct) so the
mappings populate correctly in view mode.
---
Outside diff comments:
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html`:
- Around line 60-64: The template uses optional chaining for
loanProduct.currency?.code but directly accesses
loanProduct.currency.decimalPlaces which can throw if currency is undefined;
update the binding that shows decimal places (the expression using
loanProduct.digitsAfterDecimal and loanProduct.currency.decimalPlaces) to guard
access to currency (e.g., use optional chaining or a safe fallback) so that if
loanProduct.currency is undefined and digitsAfterDecimal is nullish it doesn't
raise a runtime error while preserving the existing precedence of
loanProduct.digitsAfterDecimal over currency.decimalPlaces.
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.ts`:
- Around line 76-94: In createLoanProductCurrencyForm add Validators.min(1) to
the FormControls that the template expects min validation for: update the
'inMultiplesOf' control to include Validators.min(1), and when adding
'installmentAmountInMultiplesOf' inside the isLoanProduct branch add the same
Validators.required? or at least Validators.min(1) (and Validators.required if
the template expects required) so the form validation matches the template error
handling; reference createLoanProductCurrencyForm, controls 'inMultiplesOf' and
'installmentAmountInMultiplesOf'.
---
Nitpick comments:
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.ts`:
- Line 32: Remove the unused DestroyRef injection: delete the private destroyRef
= inject(DestroyRef); field (symbol: destroyRef) from the component class and
also remove DestroyRef from the import list so there are no unused imports
(symbol: DestroyRef, component: loan-product-currency-step.component). Ensure no
remaining references to destroyRef exist after removal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30aa1c8c-7f01-4d36-af9d-246aa558b380
📒 Files selected for processing (24)
src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.htmlsrc/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.scsssrc/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.tssrc/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.htmlsrc/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.tssrc/app/products/loan-products/working-capital/near-breach-configuration/edit-near-breach-configuration/edit-near-breach-configuration.component.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
💤 Files with no reviewable changes (1)
- src/app/products/loan-products/working-capital/near-breach-configuration/edit-near-breach-configuration/edit-near-breach-configuration.component.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.scss
🚧 Files skipped from review as they are similar to previous changes (17)
- src/assets/translations/fr-FR.json
- src/assets/translations/es-MX.json
- src/assets/translations/ne-NE.json
- src/assets/translations/lt-LT.json
- src/assets/translations/sw-SW.json
- src/assets/translations/it-IT.json
- src/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.html
- src/assets/translations/en-US.json
- src/assets/translations/es-CL.json
- src/assets/translations/de-DE.json
- src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.ts
- src/assets/translations/ko-KO.json
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts
- src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.html
- src/assets/translations/pt-PT.json
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.ts
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.html
alberto-art3ch
force-pushed
the
WEB-657/working-capital-loan-product-advanced-accounting
branch
3 times, most recently
from
bbd75a2 to
4ef5011
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (1)
301-321:⚠️ Potential issue | 🟠 MajorBug: Incorrect property access for edit values in fees/penalty form fields.
When
edit()is called, it passesrecord: AccountingMappingDTOwhich has the shape{ value: {...}, glAccount: {...} }. However,getFeesIncomeFormfieldsreadsvalues.chargeIdandvalues.incomeAccountId(lines 306, 314), which don't exist onAccountingMappingDTO. This causes the edit dialog to show incorrect default values.The same issue exists in
getPenaltyIncomeFormfields(lines 328, 336).🐛 Suggested fix
getFeesIncomeFormfields(values?: any) { const formfields: FormfieldBase[] = [ new SelectBase({ controlName: 'chargeId', label: 'Fees', - value: values ? values.chargeId : this.chargeData[0].id, + value: values ? values.value?.id : this.chargeData[0].id, options: { label: 'name', value: 'id', data: this.chargeData }, required: true, order: 1 }), new SelectBase({ controlName: 'incomeAccountId', label: 'Income Account', - value: values ? values.incomeAccountId : this.incomeAndLiabilityAccountData[0].id, + value: values ? values.glAccount?.id : this.incomeAndLiabilityAccountData[0].id, options: { label: 'name', value: 'id', data: this.incomeAndLiabilityAccountData }, required: true, order: 2 }) ]; return formfields; } getPenaltyIncomeFormfields(values?: any) { const formfields: FormfieldBase[] = [ new SelectBase({ controlName: 'chargeId', label: 'Penalty', - value: values ? values.chargeId : this.penaltyData[0].id, + value: values ? values.value?.id : this.penaltyData[0].id, options: { label: 'name', value: 'id', data: this.penaltyData }, required: true, order: 1 }), new SelectBase({ controlName: 'incomeAccountId', label: 'Income Account', - value: values ? values.incomeAccountId : this.incomeAccountData[0].id, + value: values ? values.glAccount?.id : this.incomeAccountData[0].id, options: { label: 'name', value: 'id', data: this.incomeAccountData }, required: true, order: 2 }) ]; return formfields; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts` around lines 301 - 321, getFeesIncomeFormfields is reading flat properties (values.chargeId, values.incomeAccountId) but edit() passes an AccountingMappingDTO shaped { value: {...}, glAccount: {...} }; update getFeesIncomeFormfields to pull the nested values (e.g., values.value.chargeId for the selected charge and values.glAccount.id for the income account, or the equivalent nested keys after confirming the DTO shape) and make the same change in getPenaltyIncomeFormfields so the edit dialog uses the correct nested properties from AccountingMappingDTO.src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html (1)
60-64:⚠️ Potential issue | 🟡 MinorInconsistent null-safety on currency access.
Line 60 correctly uses
loanProduct.currency?.code, but Line 64 still directly accessesloanProduct.currency.decimalPlaceswithout safe navigation. Ifcurrencyis null/undefined, this will throw.🐛 Suggested fix
<div class="flex-fill layout-row"> <span class="flex-40">{{ 'labels.inputs.Decimal Places' | translate }}:</span> - <span class="flex-60">{{ loanProduct.digitsAfterDecimal ?? loanProduct.currency.decimalPlaces }}</span> + <span class="flex-60">{{ loanProduct.digitsAfterDecimal ?? loanProduct.currency?.decimalPlaces }}</span> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html` around lines 60 - 64, The template is using safe navigation for currency.code but directly accesses loanProduct.currency.decimalPlaces which can throw when currency is null; update the binding for the decimal places display (the expression using loanProduct.digitsAfterDecimal and loanProduct.currency.decimalPlaces) to use safe navigation on currency (e.g., use loanProduct.currency?.decimalPlaces as the fallback) so the template never dereferences currency when it is null/undefined.
♻️ Duplicate comments (2)
src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts (1)
149-157:⚠️ Potential issue | 🟠 MajorProperty name casing inconsistency for buydown mappings persists.
Lines 153-154 read
this.loanProduct.buydownFeeClassificationToIncomeAccountMappings(uppercase 'F'), but line 326 readsthis.loanProduct.buydownfeeClassificationToIncomeAccountMappings(lowercase 'f'). Based on context snippet 3, the API uses lowercase 'f'. This means the view-mode initialization on lines 153-154 will always getundefined.🐛 Suggested fix
this.buydownFeeClassificationToIncomeAccountMappings = - this.loanProduct.buydownFeeClassificationToIncomeAccountMappings || []; + this.loanProduct.buydownfeeClassificationToIncomeAccountMappings || [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts` around lines 149 - 157, The view-mode initialization uses the wrong-cased API property; change the access in loan-product-summary.component.ts from this.loanProduct.buydownFeeClassificationToIncomeAccountMappings to this.loanProduct.buydownfeeClassificationToIncomeAccountMappings so it matches the API field used elsewhere (ensure the assignment to this.buydownFeeClassificationToIncomeAccountMappings uses the lowercase 'f' property); scan for other references to buydownFee/buydownfee to make them consistent with the API.src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.html (1)
67-82:⚠️ Potential issue | 🟡 MinorAdd safe navigation for penalty charge property.
Line 71 accesses
penaltyIncome.charge.namewithout safe navigation.🛡️ Suggested fix
<td mat-cell *matCellDef="let penaltyIncome"> - {{ penaltyIncome.charge.name }} + {{ penaltyIncome.charge?.name }} </td>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.html` around lines 67 - 82, The template accesses penaltyIncome.charge.name without null-safe checks, which can throw if charge is undefined; update the chargeId column cell in the loan-product-summary-adv-accounting template to use safe navigation (penaltyIncome.charge?.name) or otherwise guard against undefined so rendering won't error when penaltyIncome.charge is missing; look for the matColumnDef "chargeId", the dataSource penaltyToIncomeAccountMappings and the template variable penaltyIncome to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.html`:
- Around line 17-32: Template directly accesses
paymentFundSource.paymentType.name and paymentFundSource.fundSourceAccount.name
which can throw if those objects are null/undefined; update the bindings in the
loan-product-summary-adv-accounting template to use safe navigation (Elvis) on
paymentFundSource.paymentType and paymentFundSource.fundSourceAccount
(references: paymentChannelToFundSourceMappings,
paymentFundSource.paymentType.name, paymentFundSource.fundSourceAccount.name) so
the template renders safely when either nested object is missing.
- Around line 40-59: The cell rendering the charge name in the "chargeId" column
accesses feesIncome.charge.name without null-safe navigation; update that
template expression to use the safe navigation operator (e.g.,
feesIncome?.charge?.name) in the mat-cell with matColumnDef="chargeId" so it
matches the null-safe pattern used elsewhere (dataSource:
feeToIncomeAccountMappings, columns: feesPenaltyIncomeDisplayedColumns).
---
Outside diff comments:
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html`:
- Around line 60-64: The template is using safe navigation for currency.code but
directly accesses loanProduct.currency.decimalPlaces which can throw when
currency is null; update the binding for the decimal places display (the
expression using loanProduct.digitsAfterDecimal and
loanProduct.currency.decimalPlaces) to use safe navigation on currency (e.g.,
use loanProduct.currency?.decimalPlaces as the fallback) so the template never
dereferences currency when it is null/undefined.
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts`:
- Around line 301-321: getFeesIncomeFormfields is reading flat properties
(values.chargeId, values.incomeAccountId) but edit() passes an
AccountingMappingDTO shaped { value: {...}, glAccount: {...} }; update
getFeesIncomeFormfields to pull the nested values (e.g., values.value.chargeId
for the selected charge and values.glAccount.id for the income account, or the
equivalent nested keys after confirming the DTO shape) and make the same change
in getPenaltyIncomeFormfields so the edit dialog uses the correct nested
properties from AccountingMappingDTO.
---
Duplicate comments:
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.html`:
- Around line 67-82: The template accesses penaltyIncome.charge.name without
null-safe checks, which can throw if charge is undefined; update the chargeId
column cell in the loan-product-summary-adv-accounting template to use safe
navigation (penaltyIncome.charge?.name) or otherwise guard against undefined so
rendering won't error when penaltyIncome.charge is missing; look for the
matColumnDef "chargeId", the dataSource penaltyToIncomeAccountMappings and the
template variable penaltyIncome to make the change.
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts`:
- Around line 149-157: The view-mode initialization uses the wrong-cased API
property; change the access in loan-product-summary.component.ts from
this.loanProduct.buydownFeeClassificationToIncomeAccountMappings to
this.loanProduct.buydownfeeClassificationToIncomeAccountMappings so it matches
the API field used elsewhere (ensure the assignment to
this.buydownFeeClassificationToIncomeAccountMappings uses the lowercase 'f'
property); scan for other references to buydownFee/buydownfee to make them
consistent with the API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9dc6024e-1c2b-4938-98e7-c1b5dec40280
📒 Files selected for processing (24)
src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.htmlsrc/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.scsssrc/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.tssrc/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.htmlsrc/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.tssrc/app/products/loan-products/models/loan-product.model.tssrc/app/products/loan-products/working-capital/near-breach-configuration/edit-near-breach-configuration/edit-near-breach-configuration.component.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
💤 Files with no reviewable changes (1)
- src/app/products/loan-products/working-capital/near-breach-configuration/edit-near-breach-configuration/edit-near-breach-configuration.component.ts
✅ Files skipped from review due to trivial changes (9)
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html
- src/app/products/loan-products/models/loan-product.model.ts
- src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.scss
- src/assets/translations/es-MX.json
- src/assets/translations/ko-KO.json
- src/assets/translations/es-CL.json
- src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.ts
- src/assets/translations/it-IT.json
- src/assets/translations/de-DE.json
🚧 Files skipped from review as they are similar to previous changes (7)
- src/assets/translations/cs-CS.json
- src/assets/translations/lt-LT.json
- src/assets/translations/fr-FR.json
- src/assets/translations/en-US.json
- src/assets/translations/pt-PT.json
- src/assets/translations/sw-SW.json
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.ts
alberto-art3ch
force-pushed
the
WEB-657/working-capital-loan-product-advanced-accounting
branch
from
4ef5011 to
0b3bfd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html (1)
58-69:⚠️ Potential issue | 🟡 MinorComplete the null-safe currency fallback.
loanProduct.currency?.codefixes the first dereference, but the same block still readsloanProduct.currency.decimalPlacesandloanProduct.currency.inMultiplesOf. Ifcurrencyis optional in this view, the summary can still fail to render.Suggested fix
- <span class="flex-60">{{ loanProduct.digitsAfterDecimal ?? loanProduct.currency.decimalPlaces }}</span> + <span class="flex-60">{{ loanProduct.digitsAfterDecimal ?? loanProduct.currency?.decimalPlaces }}</span> @@ - <span class="flex-60">{{ loanProduct.inMultiplesOf ?? loanProduct.currency.inMultiplesOf }}</span> + <span class="flex-60">{{ loanProduct.inMultiplesOf ?? loanProduct.currency?.inMultiplesOf }}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html` around lines 58 - 69, The template uses loanProduct.currency without null-safety in a few places; update the conditional and fallback expressions to guard the currency dereference. Change the `@if` condition to check loanProduct.currency?.inMultiplesOf (e.g., (loanProduct.inMultiplesOf || loanProduct.currency?.inMultiplesOf)) and change the fallbacks to use the safe navigation operator: replace loanProduct.currency.decimalPlaces with loanProduct.currency?.decimalPlaces and loanProduct.currency.inMultiplesOf with loanProduct.currency?.inMultiplesOf so the view never throws when currency is undefined.src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (1)
188-224:⚠️ Potential issue | 🟠 MajorFix fee/penalty edit defaults after the
{ value, glAccount }reshape.
edit()now passes anAccountingMappingDTO, but the fee/penalty form builders still readvalues.chargeId/values.incomeAccountId. On edit, both selects open without the current selection and can overwrite a valid mapping on save.🛠️ Suggested fix
getFeesIncomeFormfields(values?: any) { const formfields: FormfieldBase[] = [ new SelectBase({ controlName: 'chargeId', label: 'Fees', - value: values ? values.chargeId : this.chargeData[0].id, + value: values ? values.value?.id : this.chargeData[0].id, options: { label: 'name', value: 'id', data: this.chargeData }, required: true, order: 1 }), new SelectBase({ controlName: 'incomeAccountId', label: 'Income Account', - value: values ? values.incomeAccountId : this.incomeAndLiabilityAccountData[0].id, + value: values ? values.glAccount?.id : this.incomeAndLiabilityAccountData[0].id, options: { label: 'name', value: 'id', data: this.incomeAndLiabilityAccountData }, required: true, order: 2 }) ]; return formfields; } getPenaltyIncomeFormfields(values?: any) { const formfields: FormfieldBase[] = [ new SelectBase({ controlName: 'chargeId', label: 'Penalty', - value: values ? values.chargeId : this.penaltyData[0].id, + value: values ? values.value?.id : this.penaltyData[0].id, options: { label: 'name', value: 'id', data: this.penaltyData }, required: true, order: 1 }), new SelectBase({ controlName: 'incomeAccountId', label: 'Income Account', - value: values ? values.incomeAccountId : this.incomeAccountData[0].id, + value: values ? values.glAccount?.id : this.incomeAccountData[0].id, options: { label: 'name', value: 'id', data: this.incomeAccountData }, required: true, order: 2 }) ]; return formfields; }Also applies to: 301-342
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts` around lines 188 - 224, The FeesIncome and PenaltyIncome edit branches still read old fields (chargeId/incomeAccountId) and must use the reshaped DTO fields used elsewhere; update the two branches in advanced-accounting-mapping-rule.component.ts to pass response.data.value.valueId into getValueData(...) and response.data.value.glAccountId into getGlAccountData(...), and apply the same replacement for the other occurrences noted (around lines 301-342) so getValueData and getGlAccountData consume valueId/glAccountId consistently.
♻️ Duplicate comments (2)
src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts (1)
153-154:⚠️ Potential issue | 🟠 MajorUse the same
buydownfee...source property here as everywhere else.This assignment reads
this.loanProduct.buydownFeeClassificationToIncomeAccountMappings, but the rest of the file still consumesthis.loanProduct.buydownfeeClassificationToIncomeAccountMappings. If the payload only exposes the lowercase-ffield, the advanced-accounting summary stays empty in view mode.🛠️ Suggested fix
- this.buydownFeeClassificationToIncomeAccountMappings = - this.loanProduct.buydownFeeClassificationToIncomeAccountMappings || []; + this.buydownFeeClassificationToIncomeAccountMappings = + this.loanProduct.buydownfeeClassificationToIncomeAccountMappings || [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts` around lines 153 - 154, The assignment uses this.loanProduct.buydownFeeClassificationToIncomeAccountMappings (capital F) but the rest of the component reads this.loanProduct.buydownfeeClassificationToIncomeAccountMappings (lowercase f), causing mismatched source data; update the assignment in loan-product-summary.component.ts so this.buydownFeeClassificationToIncomeAccountMappings is initialized from the same property the rest of the file uses (this.loanProduct.buydownfeeClassificationToIncomeAccountMappings), or defensively normalize by checking both variants (prefer the lowercase-f field) before falling back to [].src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.ts (1)
611-611:⚠️ Potential issue | 🟠 MajorClear the working-capital advanced mapping arrays when the feature is disabled.
The new subscription is still a no-op, and the
NONEbranch removes onlyadvancedAccountingRules. Any existingpaymentChannelToFundSourceMappings, fee/penalty mappings, or reason mappings stay on the form and can be submitted even after advanced accounting is turned off.🛠️ Suggested fix
if (accountingRule === 'NONE') { + this.resetWorkingCapitalAdvancedAccountingMappings(); this.loanProductAccountingForm.removeControl('advancedAccountingRules'); } else if (accountingRule === 'CASH_BASED') { ... this.loanProductAccountingForm .get('advancedAccountingRules') .valueChanges.subscribe((advancedAccountingRules: boolean) => { - if (advancedAccountingRules) { - } else { - } + if (!advancedAccountingRules) { + this.resetWorkingCapitalAdvancedAccountingMappings(); + } else { + this.ensureWorkingCapitalAdvancedAccountingMappings(); + } }); }A small helper that
setControl(..., this.formBuilder.array([]))for the five working-capital mapping arrays is enough here.Also applies to: 673-681
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.ts` at line 611, The NONE branch currently only calls loanProductAccountingForm.removeControl('advancedAccountingRules') leaving existing working-capital mapping arrays (paymentChannelToFundSourceMappings, feeToGLAccountMappings, penaltyToGLAccountMappings, reasonToGLAccountMappings, and advancedAccountingRules) populated and still submitable; update the NONE handling in the subscription (where loanProductAccountingForm.removeControl is called) to clear those five controls by calling loanProductAccountingForm.setControl('<controlName>', this.formBuilder.array([])) for each mapping control (or add a small helper method that does this for all five) so the arrays are reset when advanced accounting is disabled, and reuse the same fix in the other subscriber block around lines 673-681.
🧹 Nitpick comments (1)
src/assets/translations/lt-LT.json (1)
2091-2091: Consider keeping “Fund Source(s)” terminology consistent in Lithuanian labels.Line 2091 uses “Finansavimo šaltiniai”, while nearby accounting labels already use the “Lėšų/Fondo šaltinis” term family. Aligning terminology will make the UI more consistent.
💡 Optional wording tweak
- "Fund Sources": "Finansavimo šaltiniai", + "Fund Sources": "Lėšų šaltiniai",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/assets/translations/lt-LT.json` at line 2091, The translation for the key "Fund Sources" should be changed to match the existing terminology used elsewhere (the “Lėšų/Fondo šaltinis” family); update the value for "Fund Sources" in src/assets/translations/lt-LT.json from "Finansavimo šaltiniai" to the consistent term (e.g., "Lėšų šaltiniai" or "Fondo šaltiniai") so it aligns with neighboring accounting labels that use Lėšų/Fondo šaltinis.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts`:
- Around line 345-349: getReasonsExpenseFormfields currently returns all
accountingMappingOptions when action === 'edit', allowing duplicate mappings;
change it to allow only the option currently selected for the edited row plus
options that are not already used by other rows. In
getReasonsExpenseFormfields(action: string, values?: any) (use the values param
to get the current row's selected id, e.g. const current = values?.reasonId ||
values?.id), compute otherSelected = this.currentFormValues.filter(id => id !==
current) and then set reasonOptions = this.accountingMappingOptions.filter(item
=> item.id === current || !otherSelected.includes(item.id)); return that result
so edit behaves like add but keeps the current selection available.
In `@src/assets/translations/ko-KO.json`:
- Line 2951: The translation for the key "Charge-off Reason" should use the
existing Korean term "상각" for consistency; update the value for the "Charge-off
Reason" key in src/assets/translations/ko-KO.json from "청구 취소 사유" to a phrase
using "상각" (e.g., "상각 사유") so it matches nearby labels that already use 상각.
---
Outside diff comments:
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html`:
- Around line 58-69: The template uses loanProduct.currency without null-safety
in a few places; update the conditional and fallback expressions to guard the
currency dereference. Change the `@if` condition to check
loanProduct.currency?.inMultiplesOf (e.g., (loanProduct.inMultiplesOf ||
loanProduct.currency?.inMultiplesOf)) and change the fallbacks to use the safe
navigation operator: replace loanProduct.currency.decimalPlaces with
loanProduct.currency?.decimalPlaces and loanProduct.currency.inMultiplesOf with
loanProduct.currency?.inMultiplesOf so the view never throws when currency is
undefined.
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts`:
- Around line 188-224: The FeesIncome and PenaltyIncome edit branches still read
old fields (chargeId/incomeAccountId) and must use the reshaped DTO fields used
elsewhere; update the two branches in
advanced-accounting-mapping-rule.component.ts to pass
response.data.value.valueId into getValueData(...) and
response.data.value.glAccountId into getGlAccountData(...), and apply the same
replacement for the other occurrences noted (around lines 301-342) so
getValueData and getGlAccountData consume valueId/glAccountId consistently.
---
Duplicate comments:
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts`:
- Around line 153-154: The assignment uses
this.loanProduct.buydownFeeClassificationToIncomeAccountMappings (capital F) but
the rest of the component reads
this.loanProduct.buydownfeeClassificationToIncomeAccountMappings (lowercase f),
causing mismatched source data; update the assignment in
loan-product-summary.component.ts so
this.buydownFeeClassificationToIncomeAccountMappings is initialized from the
same property the rest of the file uses
(this.loanProduct.buydownfeeClassificationToIncomeAccountMappings), or
defensively normalize by checking both variants (prefer the lowercase-f field)
before falling back to [].
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.ts`:
- Line 611: The NONE branch currently only calls
loanProductAccountingForm.removeControl('advancedAccountingRules') leaving
existing working-capital mapping arrays (paymentChannelToFundSourceMappings,
feeToGLAccountMappings, penaltyToGLAccountMappings, reasonToGLAccountMappings,
and advancedAccountingRules) populated and still submitable; update the NONE
handling in the subscription (where loanProductAccountingForm.removeControl is
called) to clear those five controls by calling
loanProductAccountingForm.setControl('<controlName>',
this.formBuilder.array([])) for each mapping control (or add a small helper
method that does this for all five) so the arrays are reset when advanced
accounting is disabled, and reuse the same fix in the other subscriber block
around lines 673-681.
---
Nitpick comments:
In `@src/assets/translations/lt-LT.json`:
- Line 2091: The translation for the key "Fund Sources" should be changed to
match the existing terminology used elsewhere (the “Lėšų/Fondo šaltinis”
family); update the value for "Fund Sources" in
src/assets/translations/lt-LT.json from "Finansavimo šaltiniai" to the
consistent term (e.g., "Lėšų šaltiniai" or "Fondo šaltiniai") so it aligns with
neighboring accounting labels that use Lėšų/Fondo šaltinis.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53fb2ec7-4e16-418e-add5-5713f1071ff0
📒 Files selected for processing (24)
src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.htmlsrc/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.scsssrc/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.tssrc/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.htmlsrc/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.tssrc/app/products/loan-products/models/loan-product.model.tssrc/app/products/loan-products/working-capital/near-breach-configuration/edit-near-breach-configuration/edit-near-breach-configuration.component.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
💤 Files with no reviewable changes (1)
- src/app/products/loan-products/working-capital/near-breach-configuration/edit-near-breach-configuration/edit-near-breach-configuration.component.ts
✅ Files skipped from review due to trivial changes (6)
- src/assets/translations/en-US.json
- src/assets/translations/ne-NE.json
- src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.scss
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html
- src/assets/translations/de-DE.json
- src/assets/translations/es-CL.json
🚧 Files skipped from review as they are similar to previous changes (9)
- src/app/products/loan-products/models/loan-product.model.ts
- src/assets/translations/sw-SW.json
- src/assets/translations/cs-CS.json
- src/assets/translations/es-MX.json
- src/assets/translations/lv-LV.json
- src/assets/translations/fr-FR.json
- src/assets/translations/pt-PT.json
- src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.html
- src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.ts
Comment on lines
+345
to
+349
| getReasonsExpenseFormfields(action: string, values?: any) { | ||
| const reasonOptions = | ||
| action === 'edit' | ||
| ? this.accountingMappingOptions | ||
| : this.accountingMappingOptions.filter((item: any) => !this.currentFormValues.includes(item.id)); |
There was a problem hiding this comment.
Keep reason mappings unique during edit as well.
Using the full accountingMappingOptions on edit lets a user change one row to a reason that is already mapped in another row. The add flow prevents duplicates, so edit should allow only the current reason plus the still-unmapped ones.
🛠️ Suggested fix
getReasonsExpenseFormfields(action: string, values?: any) {
- const reasonOptions =
- action === 'edit'
- ? this.accountingMappingOptions
- : this.accountingMappingOptions.filter((item: any) => !this.currentFormValues.includes(item.id));
+ const currentReasonId = values?.value?.id;
+ const reasonOptions = this.accountingMappingOptions.filter(
+ (item: any) => item.id === currentReasonId || !this.currentFormValues.includes(item.id)
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts`
around lines 345 - 349, getReasonsExpenseFormfields currently returns all
accountingMappingOptions when action === 'edit', allowing duplicate mappings;
change it to allow only the option currently selected for the edited row plus
options that are not already used by other rows. In
getReasonsExpenseFormfields(action: string, values?: any) (use the values param
to get the current row's selected id, e.g. const current = values?.reasonId ||
values?.id), compute otherSelected = this.currentFormValues.filter(id => id !==
current) and then set reasonOptions = this.accountingMappingOptions.filter(item
=> item.id === current || !otherSelected.includes(item.id)); return that result
so edit behaves like add but keeps the current selection available.
| "Supported Interest Refund Types": "지원되는 이자 환불 유형", | ||
| "SUPPORTED REFUND TRANSACTIONS FOR INTEREST REFUND": "지원되는 이자 환불 유형", | ||
| "Charge-off reason": "청구 취소 사유", | ||
| "Charge-off Reason": "청구 취소 사유", |
There was a problem hiding this comment.
Use the existing Korean term for “Charge-off.”
청구 취소 사유 reads as a cancelled-charge reason, not a charge-off/write-off reason. The nearby Korean labels already use 상각, so this new key should stay consistent.
Suggested fix
- "Charge-off Reason": "청구 취소 사유",
+ "Charge-off Reason": "상각 사유",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "Charge-off Reason": "청구 취소 사유", | |
| "Charge-off Reason": "상각 사유", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/assets/translations/ko-KO.json` at line 2951, The translation for the key
"Charge-off Reason" should use the existing Korean term "상각" for consistency;
update the value for the "Charge-off Reason" key in
src/assets/translations/ko-KO.json from "청구 취소 사유" to a phrase using "상각" (e.g.,
"상각 사유") so it matches nearby labels that already use 상각.
alberto-art3ch
force-pushed
the
WEB-657/working-capital-loan-product-advanced-accounting
branch
from
0b3bfd2 to
eff515b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html (1)
60-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete the null-safe currency fallback.
Line 60 is guarded, but Line 64 still falls back to
loanProduct.currency.decimalPlaces. Ifcurrencyis missing anddigitsAfterDecimalis unset, this section still throws during render.Suggested fix
- <span class="flex-60">{{ loanProduct.digitsAfterDecimal ?? loanProduct.currency.decimalPlaces }}</span> + <span class="flex-60">{{ loanProduct.digitsAfterDecimal ?? loanProduct.currency?.decimalPlaces }}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html` around lines 60 - 64, The template uses loanProduct.currency.decimalPlaces as a fallback which can throw if currency is null; change the fallback to a null-safe access (e.g. use loanProduct.currency?.decimalPlaces) or a definitive default (e.g. '?? 0') so the expression {{ loanProduct.digitsAfterDecimal ?? loanProduct.currency.decimalPlaces }} becomes something like {{ loanProduct.digitsAfterDecimal ?? loanProduct.currency?.decimalPlaces ?? 0 }}; update the loan-product-summary template wherever digitsAfterDecimal/currency.decimalPlaces is used to avoid runtime render errors.
♻️ Duplicate comments (2)
src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts (1)
153-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the lowercase buydown mapping property here as well.
The view path reads
loanProduct.buydownFeeClassificationToIncomeAccountMappings, but the rest of this file usesloanProduct.buydownfeeClassificationToIncomeAccountMappings. Inaction === 'view', that leaves the buydown classification mappings empty even when the backend returned them.Suggested fix
this.buydownFeeClassificationToIncomeAccountMappings = - this.loanProduct.buydownFeeClassificationToIncomeAccountMappings || []; + this.loanProduct.buydownfeeClassificationToIncomeAccountMappings || [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts` around lines 153 - 154, The assignment is using the wrong-cased property name loanProduct.buydownFeeClassificationToIncomeAccountMappings so in view mode the mappings stay empty; change it to read from loanProduct.buydownfeeClassificationToIncomeAccountMappings instead and assign that to this.buydownFeeClassificationToIncomeAccountMappings (i.e., replace the RHS property name to the lowercase "b" variant so the existing variable buydownFeeClassificationToIncomeAccountMappings is populated correctly).src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (1)
349-353:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep reason mappings unique during edit too.
Edit mode still uses the full
accountingMappingOptions, so a row can be changed to a charge-off/write-off reason that is already mapped by another row. Restrict the options to the current reason plus the unused ones.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts` around lines 349 - 353, getReasonsExpenseFormfields currently returns the full accountingMappingOptions in edit mode which allows selecting reasons already used by other rows; change the edit branch to build options that include the current row's selected reason (from the values argument, e.g. values?.reasonId) plus any unused reasons by filtering accountingMappingOptions to keep items where item.id === values?.reasonId OR item.id is not in currentFormValues; update getReasonsExpenseFormfields to use that filtered list so mappings remain unique even when editing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts`:
- Around line 210-224: The edit flow is using response.data.value.chargeId /
incomeAccountId for 'FeesIncome' and 'PenaltyIncome' but edit() supplies an
AccountingMappingDTO (with value and glAccount), so preload is missing and can
overwrite mappings; modify the branches for formType 'FeesIncome' and
'PenaltyIncome' to read the AccountingMappingDTO fields
(response.data.value.value for the selection and response.data.value.glAccount
for the account) and pass those into getValueData(...) and
getGlAccountData(...), leaving other formTypes unchanged so the dialog preloads
existing mapping values.
- Around line 279-283: The payment type list in getPaymentFundSourceFormfields
currently returns the full paymentTypeData in edit mode, allowing duplicate
selections; replace the ternary so you always filter accountingMappingOptions to
exclude ids in currentFormValues, but when action === 'edit' re-include the
current row's selected payment type (use the values parameter, e.g.
values?.paymentType or values?.paymentTypeId) so the editor can keep its own
value while still preventing selection of types already used on other rows;
update references to paymentTypeData/accountingMappingOptions/currentFormValues
and values in getPaymentFundSourceFormfields accordingly.
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.html`:
- Around line 509-542: Add two mifosx-advanced-accounting-mapping-rule blocks to
the template so the UI can edit fee and penalty mappings: mirror the existing
blocks for charge-off/write-off but bind [formArray] to
feeToIncomeAccountMappings and penaltyToIncomeAccountMappings, set appropriate
[textField] values ("Fee" and "Penalty"), provide descriptive [textHeading]s,
wire the same (formChangeEvent)="formChangeEvent($event)", and pass the income
account data and mapping option inputs (e.g.,
[incomeAccountData]="incomeAccountData" and
[accountingMappingOptions]="feeOptions"/"penaltyOptions") so the TS-initialized
feeToIncomeAccountMappings and penaltyToIncomeAccountMappings are configurable
from the UI.
In `@src/assets/translations/sw-SW.json`:
- Line 2947: The translation for the JSON key "Charge-off Reason" is incorrect;
replace the value "Sababu ya malipo" with a Swahili phrase conveying
charge-off/write-off (e.g., "Sababu ya kufutwa deni" or "Sababu ya kuandika
deni") so the accounting meaning is preserved—update the value for the
"Charge-off Reason" key in src/assets/translations/sw-SW.json accordingly.
---
Outside diff comments:
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.html`:
- Around line 60-64: The template uses loanProduct.currency.decimalPlaces as a
fallback which can throw if currency is null; change the fallback to a null-safe
access (e.g. use loanProduct.currency?.decimalPlaces) or a definitive default
(e.g. '?? 0') so the expression {{ loanProduct.digitsAfterDecimal ??
loanProduct.currency.decimalPlaces }} becomes something like {{
loanProduct.digitsAfterDecimal ?? loanProduct.currency?.decimalPlaces ?? 0 }};
update the loan-product-summary template wherever
digitsAfterDecimal/currency.decimalPlaces is used to avoid runtime render
errors.
---
Duplicate comments:
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts`:
- Around line 153-154: The assignment is using the wrong-cased property name
loanProduct.buydownFeeClassificationToIncomeAccountMappings so in view mode the
mappings stay empty; change it to read from
loanProduct.buydownfeeClassificationToIncomeAccountMappings instead and assign
that to this.buydownFeeClassificationToIncomeAccountMappings (i.e., replace the
RHS property name to the lowercase "b" variant so the existing variable
buydownFeeClassificationToIncomeAccountMappings is populated correctly).
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts`:
- Around line 349-353: getReasonsExpenseFormfields currently returns the full
accountingMappingOptions in edit mode which allows selecting reasons already
used by other rows; change the edit branch to build options that include the
current row's selected reason (from the values argument, e.g. values?.reasonId)
plus any unused reasons by filtering accountingMappingOptions to keep items
where item.id === values?.reasonId OR item.id is not in currentFormValues;
update getReasonsExpenseFormfields to use that filtered list so mappings remain
unique even when editing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8fa7bdef-9af7-4d20-bd34-522963538639
📒 Files selected for processing (24)
src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.htmlsrc/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.scsssrc/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.tssrc/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.htmlsrc/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.tssrc/app/products/loan-products/models/loan-product.model.tssrc/app/products/loan-products/working-capital/near-breach-configuration/edit-near-breach-configuration/edit-near-breach-configuration.component.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
💤 Files with no reviewable changes (1)
- src/app/products/loan-products/working-capital/near-breach-configuration/edit-near-breach-configuration/edit-near-breach-configuration.component.ts
✅ Files skipped from review due to trivial changes (10)
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html
- src/app/products/loan-products/models/loan-product.model.ts
- src/assets/translations/es-CL.json
- src/assets/translations/it-IT.json
- src/assets/translations/pt-PT.json
- src/assets/translations/lt-LT.json
- src/assets/translations/de-DE.json
- src/assets/translations/ne-NE.json
- src/assets/translations/en-US.json
- src/assets/translations/fr-FR.json
🚧 Files skipped from review as they are similar to previous changes (6)
- src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.scss
- src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.html
- src/assets/translations/cs-CS.json
- src/assets/translations/ko-KO.json
- src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.ts
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.ts
Comment on lines
210
to
224
| } else if (this.formType === 'PaymentFundSource') { | ||
| updateData = { | ||
| value: this.getValueData(response.data.paymentTypeId), | ||
| glAccount: this.getGlAccountData(response.data.fundSourceAccountId) | ||
| value: this.getValueData(response.data.value.paymentTypeId), | ||
| glAccount: this.getGlAccountData(response.data.value.fundSourceAccountId) | ||
| }; | ||
| } else if (this.formType === 'FeesIncome') { | ||
| updateData = { | ||
| value: this.getValueData(response.data.chargeId), | ||
| glAccount: this.getGlAccountData(response.data.incomeAccountId) | ||
| value: this.getValueData(response.data.value.chargeId), | ||
| glAccount: this.getGlAccountData(response.data.value.incomeAccountId) | ||
| }; | ||
| } else if (this.formType === 'PenaltyIncome') { | ||
| updateData = { | ||
| value: this.getValueData(response.data.chargeId), | ||
| glAccount: this.getGlAccountData(response.data.incomeAccountId) | ||
| value: this.getValueData(response.data.value.chargeId), | ||
| glAccount: this.getGlAccountData(response.data.value.incomeAccountId) | ||
| }; |
There was a problem hiding this comment.
Fee/penalty edits don't preload the current mapping.
edit() passes an AccountingMappingDTO (value / glAccount), but the fee/penalty form builders still read chargeId and incomeAccountId. The dialog reopens with undefined/default selections and can overwrite an existing mapping even when the user only changes one field.
Suggested fix
getFeesIncomeFormfields(values?: any) {
const formfields: FormfieldBase[] = [
new SelectBase({
controlName: 'chargeId',
label: 'Fees',
- value: values ? values.chargeId : this.chargeData[0].id,
+ value: values ? values.value.id : this.chargeData[0].id,
options: { label: 'name', value: 'id', data: this.chargeData },
required: true,
order: 1
}),
new SelectBase({
controlName: 'incomeAccountId',
label: 'Income Account',
- value: values ? values.incomeAccountId : this.incomeAndLiabilityAccountData[0].id,
+ value: values ? values.glAccount?.id : this.incomeAndLiabilityAccountData[0].id,
options: { label: 'name', value: 'id', data: this.incomeAndLiabilityAccountData },
required: true,
order: 2
})
];
}
getPenaltyIncomeFormfields(values?: any) {
const formfields: FormfieldBase[] = [
new SelectBase({
controlName: 'chargeId',
label: 'Penalty',
- value: values ? values.chargeId : this.penaltyData[0].id,
+ value: values ? values.value.id : this.penaltyData[0].id,
options: { label: 'name', value: 'id', data: this.penaltyData },
required: true,
order: 1
}),
new SelectBase({
controlName: 'incomeAccountId',
label: 'Income Account',
- value: values ? values.incomeAccountId : this.incomeAccountData[0].id,
+ value: values ? values.glAccount?.id : this.incomeAccountData[0].id,
options: { label: 'name', value: 'id', data: this.incomeAccountData },
required: true,
order: 2
})
];
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts`
around lines 210 - 224, The edit flow is using response.data.value.chargeId /
incomeAccountId for 'FeesIncome' and 'PenaltyIncome' but edit() supplies an
AccountingMappingDTO (with value and glAccount), so preload is missing and can
overwrite mappings; modify the branches for formType 'FeesIncome' and
'PenaltyIncome' to read the AccountingMappingDTO fields
(response.data.value.value for the selection and response.data.value.glAccount
for the account) and pass those into getValueData(...) and
getGlAccountData(...), leaving other formTypes unchanged so the dialog preloads
existing mapping values.
Comment on lines
+279
to
+283
| getPaymentFundSourceFormfields(action: string, values?: any) { | ||
| const paymentTypeOptions = | ||
| action === 'edit' | ||
| ? this.paymentTypeData | ||
| : this.accountingMappingOptions.filter((item: any) => !this.currentFormValues.includes(item.id)); |
There was a problem hiding this comment.
Keep payment types unique during edit too.
Add mode filters already-used payment types, but edit mode exposes the full list again. That lets one row be changed to a payment type that is already mapped somewhere else.
Suggested fix
- const paymentTypeOptions =
- action === 'edit'
- ? this.paymentTypeData
- : this.accountingMappingOptions.filter((item: any) => !this.currentFormValues.includes(item.id));
+ const currentPaymentTypeId = values?.value?.id;
+ const usedPaymentTypeIds = this.currentFormValues.filter((id) => id !== currentPaymentTypeId);
+ const paymentTypeOptions = this.accountingMappingOptions.filter(
+ (item: any) => item.id === currentPaymentTypeId || !usedPaymentTypeIds.includes(item.id)
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts`
around lines 279 - 283, The payment type list in getPaymentFundSourceFormfields
currently returns the full paymentTypeData in edit mode, allowing duplicate
selections; replace the ternary so you always filter accountingMappingOptions to
exclude ids in currentFormValues, but when action === 'edit' re-include the
current row's selected payment type (use the values parameter, e.g.
values?.paymentType or values?.paymentTypeId) so the editor can keep its own
value while still preventing selection of types already used on other rows;
update references to paymentTypeData/accountingMappingOptions/currentFormValues
and values in getPaymentFundSourceFormfields accordingly.
Comment on lines
+509
to
+542
| @if (loanProductAccountingForm.value.advancedAccountingRules) { | ||
| <div class="flex-100 layout-row-wrap responsive-column"> | ||
| <mifosx-advanced-accounting-mapping-rule | ||
| class="flex-100 m-t-10" | ||
| [textField]="'Fund Sources'" | ||
| [formType]="'PaymentFundSource'" | ||
| [formArray]="paymentChannelToFundSourceMappings" | ||
| [textHeading]="'Configure Fund Sources for Payment Channels'" | ||
| [paymentTypeData]="paymentTypeData" | ||
| [assetAccountData]="assetAccountData" | ||
| [accountingMappingOptions]="paymentTypeData" | ||
| (formChangeEvent)="formChangeEvent($event)" | ||
| ></mifosx-advanced-accounting-mapping-rule> | ||
| <mifosx-advanced-accounting-mapping-rule | ||
| class="flex-100 m-t-10" | ||
| [textField]="'Charge-off Reason'" | ||
| [formType]="'ChargeOffReasonExpense'" | ||
| [formArray]="chargeOffReasonToExpenseAccountMappings" | ||
| [textHeading]="'Map Charge-off reasons to Expense accounts'" | ||
| [expenseAccountData]="expenseAccountData" | ||
| [accountingMappingOptions]="chargeOffReasonOptions" | ||
| (formChangeEvent)="formChangeEvent($event)" | ||
| ></mifosx-advanced-accounting-mapping-rule> | ||
| <mifosx-advanced-accounting-mapping-rule | ||
| class="flex-100 m-t-10" | ||
| [textField]="'WriteOff Reason'" | ||
| [formType]="'WriteOffReasonToExpense'" | ||
| [formArray]="writeOffReasonsToExpenseMappings" | ||
| [textHeading]="'WriteOff reasons to Expense accounts'" | ||
| [expenseAccountData]="expenseAccountData" | ||
| [accountingMappingOptions]="writeOffReasonOptions" | ||
| (formChangeEvent)="formChangeEvent($event)" | ||
| ></mifosx-advanced-accounting-mapping-rule> | ||
| </div> |
There was a problem hiding this comment.
Working-capital advanced accounting is still missing fee and penalty mappings.
This branch only renders fund-source, charge-off-reason, and write-off-reason editors. The TS layer already initializes and serializes feeToIncomeAccountMappings and penaltyToIncomeAccountMappings, so there is still no way to configure those mappings from the UI for working-capital products.
Suggested fix
`@if` (loanProductAccountingForm.value.advancedAccountingRules) {
<div class="flex-100 layout-row-wrap responsive-column">
<mifosx-advanced-accounting-mapping-rule
class="flex-100 m-t-10"
[textField]="'Fund Sources'"
[formType]="'PaymentFundSource'"
[formArray]="paymentChannelToFundSourceMappings"
[textHeading]="'Configure Fund Sources for Payment Channels'"
[paymentTypeData]="paymentTypeData"
[assetAccountData]="assetAccountData"
[accountingMappingOptions]="paymentTypeData"
(formChangeEvent)="formChangeEvent($event)"
></mifosx-advanced-accounting-mapping-rule>
+ <mifosx-advanced-accounting-mapping-rule
+ class="flex-100 m-t-10"
+ [textField]="'Fees'"
+ [formType]="'FeesIncome'"
+ [formArray]="feeToIncomeAccountMappings"
+ [textHeading]="'Map Fees to Specific Income Accounts'"
+ [chargeData]="chargeData"
+ [incomeAndLiabilityAccountData]="incomeAndLiabilityAccountData"
+ [accountingMappingOptions]="chargeData"
+ (formChangeEvent)="formChangeEvent($event)"
+ ></mifosx-advanced-accounting-mapping-rule>
+ <mifosx-advanced-accounting-mapping-rule
+ class="flex-100 m-t-10"
+ [textField]="'Penalty'"
+ [formType]="'PenaltyIncome'"
+ [formArray]="penaltyToIncomeAccountMappings"
+ [textHeading]="'Map Penalties to Specific Income Accounts'"
+ [penaltyData]="penaltyData"
+ [incomeAccountData]="incomeAccountData"
+ [accountingMappingOptions]="penaltyData"
+ (formChangeEvent)="formChangeEvent($event)"
+ ></mifosx-advanced-accounting-mapping-rule>
<mifosx-advanced-accounting-mapping-rule
class="flex-100 m-t-10"
[textField]="'Charge-off Reason'"
[formType]="'ChargeOffReasonExpense'"
[formArray]="chargeOffReasonToExpenseAccountMappings"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.html`
around lines 509 - 542, Add two mifosx-advanced-accounting-mapping-rule blocks
to the template so the UI can edit fee and penalty mappings: mirror the existing
blocks for charge-off/write-off but bind [formArray] to
feeToIncomeAccountMappings and penaltyToIncomeAccountMappings, set appropriate
[textField] values ("Fee" and "Penalty"), provide descriptive [textHeading]s,
wire the same (formChangeEvent)="formChangeEvent($event)", and pass the income
account data and mapping option inputs (e.g.,
[incomeAccountData]="incomeAccountData" and
[accountingMappingOptions]="feeOptions"/"penaltyOptions") so the TS-initialized
feeToIncomeAccountMappings and penaltyToIncomeAccountMappings are configurable
from the UI.
| "Supported Interest Refund Types": "Aina za Urejeshaji wa Riba Zinazotumika", | ||
| "SUPPORTED REFUND TRANSACTIONS FOR INTEREST REFUND": "Aina za kurejesha riba zinazotumika", | ||
| "Charge-off reason": "Sababu ya malipo", | ||
| "Charge-off Reason": "Sababu ya malipo", |
There was a problem hiding this comment.
Fix mistranslation for “Charge-off Reason”.
Line 2947 currently translates to “reason for payment,” which changes accounting meaning in the UI header. Please use a charge-off/write-off equivalent instead.
💡 Suggested fix
- "Charge-off Reason": "Sababu ya malipo",
+ "Charge-off Reason": "Sababu ya kufuta mkopo",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/assets/translations/sw-SW.json` at line 2947, The translation for the
JSON key "Charge-off Reason" is incorrect; replace the value "Sababu ya malipo"
with a Swahili phrase conveying charge-off/write-off (e.g., "Sababu ya kufutwa
deni" or "Sababu ya kuandika deni") so the accounting meaning is
preserved—update the value for the "Charge-off Reason" key in
src/assets/translations/sw-SW.json accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Advance accounting rules mapping can be done from UI for Working Cpi. Similar to what we have in term loan product except capitalized income and buydown fees mapping.
Related issues and discussion
WEB-657
Screenshots, if any
Screen.Recording.2026-04-29.at.10.36.50.AM.mov
Checklist
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
Localization