WEB-657: Loan product advanced accounting no data issue - #3688
Merged
adamsaghy merged 1 commit intoJul 9, 2026
Merged
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Summary rendering and layout src/app/products/loan-products/common/loan-product-summary/.../loan-product-summary-adv-accounting.component.html, src/app/products/loan-products/common/loan-product-summary/.../loan-product-summary-adv-accounting.component.scss |
Income account cells now show glCode before the account name, the charge-off reason header key changes capitalization, and the summary tables use fixed column sizing. |
FeesIncome lookup and helper src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts |
FeesIncome and PenaltyIncome now resolve glAccount from the intended account lists, the form field defaults read from values.value.id and values.glAccount.id, and a list-scoped GL account helper is added. |
Mapping rule table sizing src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.scss |
The mapping-rule table uses fixed layout with explicit first- and last-column widths. |
Charge-off reason label alignment src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.html, src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/loan-product-accounting-step.component.ts |
The charge-off reason label text is updated in the accounting step template and form field definition. |
Installment multiples default src/app/products/loan-products/loan-product-stepper/loan-product-currency-step/loan-product-currency-step.component.ts |
installmentAmountInMultiplesOf now patches to 1 when the template value is zero, null, or undefined. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Suggested reviewers: adamsaghy
🚥 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 matches the PR’s main focus on fixing the loan product advanced accounting no-data issue. |
| 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. |
✨ 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.
Comment @coderabbitai help to get the list of available commands.
![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
July 2, 2026 02:25
c8e59b4 to
ec4bbf4
Compare
alberto-art3ch
force-pushed
the
WEB-657/working-capital-loan-product-adv-accounting
branch
from
July 3, 2026 12:50
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
alberto-art3ch
force-pushed
the
WEB-657/working-capital-loan-product-adv-accounting
branch
from
July 8, 2026 12:52
8bafc1a to
42e3621
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts (1)
263-263: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist
concatoutside the loop.
incomeAccountData.concat(liabilityAccountData)is called inside theforEachcallback, creating a new array on each iteration. Compute it once before the loop, consistent with howassetAndLiabilityAccountDatais handled at line 179.♻️ Proposed refactor
this.feeToIncomeAccountMappings = []; + const incomeAndLiabilityAccountData = incomeAccountData.concat(liabilityAccountData); if (this.loanProduct.feeToIncomeAccountMappings?.length > 0) { this.loanProduct.feeToIncomeAccountMappings.forEach((m: any) => { const incomeAccountId = m.incomeAccountId ?? m.glAccount?.id; const chargeId = m.chargeId ?? m.value?.id; this.feeToIncomeAccountMappings.push({ - incomeAccount: this.glAccountLookUp(incomeAccountId, incomeAccountData.concat(liabilityAccountData)), + incomeAccount: this.glAccountLookUp(incomeAccountId, incomeAndLiabilityAccountData), charge: this.chargeLookUp(chargeId, this.loanProductsTemplate.chargeOptions) }); }); }🤖 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/common/loan-product-summary/loan-product-summary.component.ts` at line 263, Hoist the repeated income/liability array merge out of the `forEach` in `loan-product-summary.component.ts`: `incomeAccountData.concat(liabilityAccountData)` is being recreated on every iteration inside the callback that builds the account summaries. Create the combined array once before the loop, similar to `assetAndLiabilityAccountData`, and then pass that precomputed value into `glAccountLookUp` for `incomeAccount` to avoid unnecessary allocations.
🤖 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.
Nitpick comments:
In
`@src/app/products/loan-products/common/loan-product-summary/loan-product-summary.component.ts`:
- Line 263: Hoist the repeated income/liability array merge out of the `forEach`
in `loan-product-summary.component.ts`:
`incomeAccountData.concat(liabilityAccountData)` is being recreated on every
iteration inside the callback that builds the account summaries. Create the
combined array once before the loop, similar to `assetAndLiabilityAccountData`,
and then pass that precomputed value into `glAccountLookUp` for `incomeAccount`
to avoid unnecessary allocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10f6106b-9788-47a4-ba9a-1abcd47fc7e2
📒 Files selected for processing (8)
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.component.tssrc/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 (4)
- 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.html
- 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/loan-product-accounting-step.component.ts
🚧 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/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts
2 tasks
2 tasks
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