Update loans-account-terms-step.component.ts#3723
Open
iaitsupport wants to merge 1 commit into
Open
Conversation
`Angular`'s `SimpleChanges` object tells us, for the `loansAccountProductTemplate` input specifically, whether this is the **first** time it changed (`isFirstChange()`). Behavior now: | Scenario | `isInitialLoad` | Data used | |---|---|---| | Edit page opens for the first time | `true` | `loansAccountTemplate` (the loan's own current data) — same as before, unchanged behavior | | User picks a different product afterward | `false` | `loansAccountProductTemplate` (the freshly-fetched template for the newly selected product) — **this is the fix** |
|
👋 Hi @iaitsupport — thank you for your pull request. This PR is currently blocked because we do not have a Contributor License Agreement (CLA) on file for your GitHub account. To get unblocked:
|
mifos-cla-check
Bot
added
the
cla-required
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Initial terms selection src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts |
ngOnChanges applies loansAccountTemplate only on the initial product-template change and otherwise preserves loansAccountProductTemplate in loansAccountTermsData. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Possibly related PRs
- openMF/web-app#3661: Both modify the loan product/template change flow.
- openMF/web-app#3701: Both modify
LoansAccountTermsStepComponent.ngOnChangesinitialization and update logic.
Suggested reviewers: alberto-art3ch, adamsaghy, jaysoni1
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title is too generic and only names a file, so it doesn't convey the actual behavior change. | Use a specific title that mentions the loan terms step behavior fix, such as preserving initial edit load while honoring later product changes. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| 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. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ 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: 1
🤖 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/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts`:
- Around line 207-213: The loansAccountProductTemplate branch currently captures
working-capital products and updates loansAccountTermsData on unrelated input
changes. In the relevant input-change handler, gate this branch with
loanProductService.isLoanProduct, require changes['loansAccountProductTemplate']
before assigning loansAccountTermsData, and preserve the initial-load loan
template override within that change-specific block so the working-capital
else-if remains reachable.
🪄 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: 083bd04f-ffb8-4ba9-80d2-b8eb065c349b
📒 Files selected for processing (1)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts
Comment on lines
+207
to
+213
| if (this.loansAccountProductTemplate) { | ||
| const templateChange = changes['loansAccountProductTemplate']; | ||
| const isInitialLoad = !templateChange || templateChange.isFirstChange(); | ||
| this.loansAccountTermsData = this.loansAccountProductTemplate; | ||
| if (isInitialLoad && this.loanId != null && this.loansAccountTemplate?.accountNo) { | ||
| this.loansAccountTermsData = this.loansAccountTemplate; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target component and nearby relevant symbols first
ast-grep outline src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts --view expanded
printf '\n--- related component search ---\n'
rg -n "isLoanProduct|isWorkingCapital|loansAccountProductTemplate|loansAccountTermsData|ngOnChanges" src/app/loans -g '*.ts'Repository: openMF/web-app
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of the component with line numbers.
sed -n '180,380p' src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts | cat -nRepository: openMF/web-app
Length of output: 12744
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the analogous creation component for how it branches on product type.
sed -n '1,260p' src/app/loans/create-loans-account/create-loans-account.component.ts | cat -nRepository: openMF/web-app
Length of output: 11861
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts')
text = p.read_text()
for start in [180, 200, 320, 340]:
print(f"\n--- lines {start}-{start+40} ---")
for i, line in enumerate(text.splitlines(), 1):
if start <= i <= start + 40:
print(f"{i:4}: {line}")
PYRepository: openMF/web-app
Length of output: 10482
Restrict this branch to loan products and only run it on loansAccountProductTemplate changes.
if (this.loansAccountProductTemplate) makes the working-capital else if unreachable, and !templateChange resets loansAccountTermsData on unrelated @Input() updates. Gate this on this.loanProductService.isLoanProduct and move the assignment inside if (templateChange).
🤖 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/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts`
around lines 207 - 213, The loansAccountProductTemplate branch currently
captures working-capital products and updates loansAccountTermsData on unrelated
input changes. In the relevant input-change handler, gate this branch with
loanProductService.isLoanProduct, require changes['loansAccountProductTemplate']
before assigning loansAccountTermsData, and preserve the initial-load loan
template override within that change-specific block so the working-capital
else-if remains reachable.
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
1 participant
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.
Problem
When editing an existing loan account and changing the selected Loan Product
in the Details step, the Terms step does not update to reflect the newly
selected product — it keeps showing the original loan's data.
So the bug is not a missing/broken API call — it's in how the response is
(mis)handled afterward.
Root cause
LoansAccountTermsStepComponent.ngOnChangescouldn't distinguish between twocases that both fire the same lifecycle hook:
data is correct), and
fetched product template should be used instead).
this.loanId != null && this.loansAccountTemplate?.accountNois true for theentire lifetime of the edit page, regardless of which product is currently
selected. So every time a new product template arrived, this branch
immediately discarded it and fell back to the loan's original (stale) data —
the Terms form just kept re-patching the same old values.
This has been present since #WEB-711 (Working Capital product configuration,
2026-02-15). Notably, the sibling Working Capital branch a few lines below
already guards against this exact scenario via
SimpleChanges.isFirstChange()/ previous-vs-current product id comparison — the plain-loan branch above it
never received the equivalent fix.
Fix
Use
SimpleChangesto detect whether this is the first timeloansAccountProductTemplatechanged (initial load) versus a later change(user picked a different product), and only fall back to the stale loan data
on the initial load:
No other logic in the method changed — all downstream consumers of
loansAccountTermsData(currency resolution, form patching, attribute-overridedisabling, etc.) now simply operate on the correct source depending on which
case applies.
Testing
loan's own current data on load (unchanged behavior).
to reflect the newly selected product's template (previously stayed on the
old product's terms).
Summary by CodeRabbit