WEB-657: Loan product advanced accounting no data issue#3688
Open
alberto-art3ch wants to merge 1 commit into
Open
WEB-657: Loan product advanced accounting no data issue#3688alberto-art3ch wants to merge 1 commit into
alberto-art3ch wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughLoan product advanced accounting summaries now display GL codes with account names, charge-off labels use updated capitalization, the advanced accounting mapping rule uses list-scoped GL account lookup, and the currency step now defaults installment multiples to 1. ChangesLoan product accounting and currency step updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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-currency-step/loan-product-currency-step.component.ts (1)
67-71: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve
0/unset instead of rewriting it to1.Line 71 now converts both a valid
0and an optional missing value into1. BecauseloanProductCurrencylater returnsgetRawValue()unchanged, editing an older product/template withinstallmentAmountInMultiplesOf = 0(or unset) will silently persist1even if the user never touched this field. It also changes the summary behavior, since1is truthy but0is not.Suggested fix
this.loanProductCurrencyForm.patchValue({ installmentAmountInMultiplesOf: - this.loanProductsTemplate.installmentAmountInMultiplesOf === 0 || - this.loanProductsTemplate.installmentAmountInMultiplesOf === undefined || - this.loanProductsTemplate.installmentAmountInMultiplesOf === null - ? 1 - : this.loanProductsTemplate.installmentAmountInMultiplesOf + this.loanProductsTemplate.installmentAmountInMultiplesOf === undefined || + this.loanProductsTemplate.installmentAmountInMultiplesOf === null + ? 0 + : this.loanProductsTemplate.installmentAmountInMultiplesOf });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.ts` around lines 67 - 71, The defaulting logic in loanProductCurrency within loan-product-currency-step.component.ts is rewriting a valid 0 and an unset value into 1, which changes existing data on load. Update the initialization path so installmentAmountInMultiplesOf preserves 0 and missing values as-is (or only applies a default when the user explicitly sets it), and make sure the loanProductCurrency form summary/getRawValue flow still reflects the original value from loanProductsTemplate without silently coercing it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
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 78-79: The penalty account display in
loan-product-summary-adv-accounting.component.html still renders empty
parentheses when penaltyIncome.incomeAccount is missing because only the inner
bindings are guarded. Update the template around the penaltyIncome.incomeAccount
render so the GL code and name are shown together only when the account exists,
using the same conditional pattern already used in the fees table or by guarding
the parentheses on glCode.
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 138-141: The edit flow in
advanced-accounting-mapping-rule.component.ts is still resolving the fee income
account with getGlAccountData instead of the same
incomeAndLiabilityAccountData-backed lookup used by add(), which can turn a
liability-based FeesIncome mapping into glAccount: null. Update edit() to use
the same getGlAccountDataFromList path for incomeAccountId so the round-tripped
mapping stays complete when sent back to the parent form.
---
Outside diff comments:
In
`@src/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.ts`:
- Around line 67-71: The defaulting logic in loanProductCurrency within
loan-product-currency-step.component.ts is rewriting a valid 0 and an unset
value into 1, which changes existing data on load. Update the initialization
path so installmentAmountInMultiplesOf preserves 0 and missing values as-is (or
only applies a default when the user explicitly sets it), and make sure the
loanProductCurrency form summary/getRawValue flow still reflects the original
value from loanProductsTemplate without silently coercing it.
🪄 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: ba36f917-2784-4e9d-a854-a643bd68a001
📒 Files selected for processing (5)
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/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.scsssrc/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-currency-step/loan-product-currency-step.component.ts
alberto-art3ch
force-pushed
the
WEB-657/working-capital-loan-product-adv-accounting
branch
from
c8e59b4 to
ec4bbf4
Compare
alberto-art3ch
force-pushed
the
WEB-657/working-capital-loan-product-adv-accounting
branch
from
ec4bbf4 to
8bafc1a
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 (1)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (1)
144-149: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPenaltyIncome
add()/edit()GL lookup mismatch.
edit()now resolves PenaltyIncome's income account viagetGlAccountDataFromList(incomeAccountId, this.incomeAccountData)(list-scoped), butadd()at Line 147 still usesgetGlAccountData(incomeAccountId), which searches acrossincomeAccountData,expenseAccountData,assetAccountData, andliabilityAccountData. This reintroduces the same class of inconsistency that the FeesIncome fix in this PR just resolved —add()andedit()can now diverge for identical input, since the cross-list search could theoretically match an id in a different account category before/instead of the scoped list.🐛 Proposed fix to align `add()` with `edit()`
} else if (this.formType === 'PenaltyIncome') { const addData: AccountingMappingDTO = { value: this.getValueData(response.data.value.chargeId), - glAccount: this.getGlAccountData(response.data.value.incomeAccountId) + glAccount: this.getGlAccountDataFromList(response.data.value.incomeAccountId, this.incomeAccountData) };Also applies to: 227-231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. 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 144 - 149, PenaltyIncome lookup is inconsistent between add() and edit() because addTableData still uses getGlAccountData() while edit() uses the list-scoped getGlAccountDataFromList() for incomeAccountId. Update the PenaltyIncome branch in advanced-accounting-mapping-rule.component.ts so add() resolves the GL account from this.incomeAccountData via getGlAccountDataFromList(), matching the edit() path and keeping both flows aligned for identical inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
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 317-325: The edit form prefill logic in the advanced accounting
mapping builder can crash when `AccountingMappingDTO.glAccount` is null. Update
the affected `SelectBase` initializers in
`advanced-accounting-mapping-rule.component.ts` to guard `values.glAccount` with
optional chaining and keep the existing default fallback from
`incomeAndLiabilityAccountData[0].id` (and any similar `values.glAccount.id`
usages in the same form builder), so opening an existing mapping without a
resolved account uses the default account instead of throwing.
---
Outside diff 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 144-149: PenaltyIncome lookup is inconsistent between add() and
edit() because addTableData still uses getGlAccountData() while edit() uses the
list-scoped getGlAccountDataFromList() for incomeAccountId. Update the
PenaltyIncome branch in advanced-accounting-mapping-rule.component.ts so add()
resolves the GL account from this.incomeAccountData via
getGlAccountDataFromList(), matching the edit() path and keeping both flows
aligned for identical inputs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb4e74e5-f9a3-450b-a130-f6447fc56e96
📒 Files selected for processing (7)
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/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.scsssrc/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.ts
✅ Files skipped from review due to trivial changes (3)
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.scss
- 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
🚧 Files skipped from review as they are similar to previous changes (3)
- 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/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.ts
- src/app/products/loan-products/common/loan-product-summary/loan-product-summary-adv-accounting/loan-product-summary-adv-accounting.component.scss
Comment on lines
+317
to
+325
| 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, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target component around the reported lines.
FILE="src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts"
wc -l "$FILE"
sed -n '280,380p' "$FILE"
# Find the DTO/type definition for AccountingMappingDTO and GLAccount.
rg -n "interface AccountingMappingDTO|type AccountingMappingDTO|class AccountingMappingDTO|glAccount" src/app -g '!**/node_modules/**'Repository: openMF/web-app
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts"
# Show surrounding code for the helper methods and their call sites.
sed -n '1,220p' "$FILE"
printf '\n--- call sites ---\n'
rg -n "getFeesIncomeFormfields|getPenaltyIncomeFormfields|getPaymentFundSourceFormfields|getReasonsExpenseFormfields|advancedAccountingMappingRule" "$FILE" src/app -g '!**/node_modules/**'
# Find the DTO/type definitions.
printf '\n--- types ---\n'
rg -n "AccountingMappingDTO|GLAccount" src/app -g '!**/node_modules/**' | head -n 80Repository: openMF/web-app
Length of output: 29000
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target component in a narrower range.
FILE="src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts"
sed -n '1,220p' "$FILE"
# Locate the AccountingMappingDTO type and its glAccount field.
printf '\n--- AccountingMappingDTO definition ---\n'
rg -n -A8 -B4 "interface AccountingMappingDTO|type AccountingMappingDTO|class AccountingMappingDTO" src/app -g '!**/node_modules/**'
# Locate exact references to glAccount typing in accounting-related DTOs.
printf '\n--- GLAccount-related types ---\n'
rg -n -A4 -B4 "glAccount: .*null|glAccount\\??:" src/app -g '!**/node_modules/**' | head -n 120Repository: openMF/web-app
Length of output: 20676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the exact DTO definition and any nullable glAccount field.
rg -n -A6 -B6 "AccountingMappingDTO" src/app -g '!**/node_modules/**'
rg -n -A6 -B6 "glAccount.*null|glAccount:.*GLAccount" src/app -g '!**/node_modules/**' | head -n 120Repository: openMF/web-app
Length of output: 47649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts"
# Show the rest of the component where these helpers are invoked.
sed -n '220,438p' "$FILE"
# Check whether edit flows use stored values from API/template objects.
printf '\n--- edit/data flow ---\n'
rg -n -A5 -B5 "edit|values\\b|formfields: this\\.get.*Formfields" "$FILE"Repository: openMF/web-app
Length of output: 14488
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find likely model files for accounting mappings.
fd -i "accounting.*mapping|mapping.*dto|glaccount" src/app
# Then search those model directories for the DTO definition.
rg -n -A8 -B4 "AccountingMappingDTO|glAccount" src/app/models src/app -g '!**/node_modules/**' | head -n 200Repository: openMF/web-app
Length of output: 22430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for nullable glAccount declarations in model files.
rg -n "glAccount: .*null|glAccount\\?: .*GLAccount|glAccount\\s*[:?]\\s*GLAccount" src/app -g '*.ts' | head -n 200Repository: openMF/web-app
Length of output: 828
Guard nullable glAccount before prefilling edit values
AccountingMappingDTO.glAccount can be null, so values.glAccount.id will throw when an existing mapping has no resolved account. Use optional chaining with the fallback in all affected form builders so edit opens with the default account instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
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 317 - 325, The edit form prefill logic in the advanced accounting
mapping builder can crash when `AccountingMappingDTO.glAccount` is null. Update
the affected `SelectBase` initializers in
`advanced-accounting-mapping-rule.component.ts` to guard `values.glAccount` with
optional chaining and keep the existing default fallback from
`incomeAndLiabilityAccountData[0].id` (and any similar `values.glAccount.id`
usages in the same form builder), so opening an existing mapping without a
resolved account uses the default account instead of throwing.
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
Loan product advanced accounting no data issue, the data not was added, then it was not saved
Related issues and discussion
WEB-657
Screenshots
Screen.Recording.2026-06-25.at.11.59.03.PM.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