WEB-813: Working Capital loan account modification#3435
Merged
adamsaghy merged 1 commit intoopenMF:devfrom Mar 31, 2026
Merged
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 |
|---|---|
Service Layer src/app/loans/loans.service.ts |
Renamed getWorkingCapitalLoannDetails → getWorkingCapitalLoanDetails; added productType parameter to getDelinquencyTags/getDelinquencyActions and to updateLoansAccount(productType, loanId, loanData) and adjusted request paths accordingly. |
Common resolvers & base src/app/loans/common-resolvers/loan-base.resolver.ts, src/app/loans/common-resolvers/*.resolver.ts |
Added LoanBaseResolver (productType BehaviorSubject, helpers and path getters). Updated many resolvers to extend it, call initialize(route), and branch service calls using loanAccountPath/productType; added numeric checks before some delinquency requests. |
Loan details & account/template resolvers src/app/loans/common-resolvers/loan-details.resolver.ts, src/app/loans/common-resolvers/loans-account-and-template.resolver.ts |
Fixed working-capital service method call (getWorkingCapitalLoanDetails) and made resolver choose between loan vs working-capital endpoints via LoanProductService. |
Edit Loans Account Component src/app/loans/edit-loans-account/edit-loans-account.component.ts |
Reordered initialization, add conditional handling for working-capital templates, added getWorkingCapitalLoanProductTemplate, updated setTemplate, implemented submitWorkingCapitalProduct() to build/clean payload and call updateLoansAccount(productType, loanId, ...), and adjusted submitLoanProduct() call to include productPath. |
Stepper components — details/preview/terms src/app/loans/loans-account-stepper/... |
Refactored product/template selection into getProductTemplate(emitEvent), safer product list init, auto-select logic, working-capital-specific form patches and validators, UI template changes for fund rendering, adjusted Cancel button navigation to include queryParams: { productType }, and numeric input constraints for working-capital term fields. |
Loans view & preview helpers src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.ts, src/app/loans/loans-view/loan-delinquency-tags-tab/... |
Added fundName(fundId) helper; some components now extend loan product base and use loanAccountPath for delinquency actions. |
Validators & directives src/app/shared/validators/amount-value.validator.ts, src/app/directives/positive-number.directive.ts, src/app/directives/directives.module.ts |
Added amountValueValidator() enforcing digit/decimal limits; introduced PositiveNumberDirective ([mifosxPositiveNumber]) and exported it from DirectivesModule. |
Error handling src/app/core/http/error-handler.interceptor.ts |
Improved 400 error message parsing: extract optional parameterName, normalize escaped dots in server messages, and prefix bad-request alerts with parameter name when present. |
Translations src/assets/translations/* |
Added commons["higher than allowed"] key across multiple locale JSON files (en, cs, de, es-CL, es-MX, fr, it, ko, lt, lv, ne, pt-PT, sw). |
Sequence Diagram(s)
sequenceDiagram
participant Client
participant Resolver
participant LoanProductService
participant LoansService
participant Server
Client->>Resolver: route resolve request
Resolver->>LoanProductService: check productType / isLoanProduct
alt Standard loan product
Resolver->>LoansService: getLoansAccountAndTemplateResource(loanId)
LoansService->>Server: GET /loans/{loanId} (associations=all)
else Working-capital product
Resolver->>LoansService: getWorkingCapitalLoanDetails(loanId)
LoansService->>Server: GET /working-capital-loans/{loanId}?associations=all
end
Server-->>LoansService: loan payload
LoansService-->>Resolver: Observable<loan>
Resolver-->>Client: resolved data
sequenceDiagram
participant UIComponent
participant LoanProductService
participant LoansService
participant Server
UIComponent->>UIComponent: build payload (format dates, remove omitted attrs)
UIComponent->>LoanProductService: read loanAccountPath / productType
UIComponent->>LoansService: updateLoansAccount(productType, loanId, payload)
LoansService->>Server: PUT /{productType}/{loanId}
Server-->>LoansService: success response
LoansService-->>UIComponent: Observable response
UIComponent->>UIComponent: navigate back with productType query param
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- WEB-711: Working Capital product #3159: Adds/extends working-capital product support and product-aware service/resolver changes (closely related code paths).
- WEB-657: WorkingCapital Loan approve or reject actions #3416: Modifies loans-account-terms-step and working-capital term behavior (overlapping template/validation changes).
- WEB-813: Working Capital loan account creation #3257: Overlaps on working-capital loan detail method naming and resolver/service edits.
Suggested reviewers
- adamsaghy
- IOhacker
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and specifically references the main objective: enabling modification of a submitted working capital loan account, which aligns with the primary changes across resolvers, services, and components. |
✏️ 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.
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: 3
🤖 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/loans/common-resolvers/loans-account-and-template.resolver.ts`:
- Around line 32-37: The resolver's resolve method must derive productType from
the route instead of relying on existing loanProductService state; call
LoanProductBaseComponent.resolveProductTypeDefault(route, 'loan') to get the
productType, invoke this.loanProductService.initialize(productType), then use
this.loanProductService.isLoanProduct to choose between
this.loansService.getLoansAccountAndTemplateResource(loanId) and
this.loansService.getWorkingCapitalLoanDetails(loanId); update the
resolve(route: ActivatedRouteSnapshot) implementation accordingly.
In `@src/app/loans/edit-loans-account/edit-loans-account.component.ts`:
- Around line 84-93: The edit flag and the submitted loan account object are
being overwritten when you set loansAccountProductTemplate =
data.loansAccountAndTemplate after calling getWorkingCapitalLoanProductTemplate;
instead, retain the existing edit-shaped object (the one you set with
this.loansAccountAndTemplate['edit'] = true) and merge only the
template/metadata/options from the service response into it. Update the code
paths around the assignments to loansAccountProductTemplate (including the
blocks using loanProductService.isLoanProduct and isWorkingCapital and the
follow-up getWorkingCapitalLoanProductTemplate call) so you copy/assign template
fields onto the existing this.loansAccountProductTemplate object (or use
Object.assign/ spread) rather than replacing the entire object; apply the same
merge approach to the similar assignments elsewhere that touch
loansAccountProductTemplate (the other occurrences noted) so the edit flag and
account-specific fields (e.g., client.id, product.id) are preserved.
In
`@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts`:
- Around line 320-335: The patch is using the wrong form control name: in
loans-account-terms-step.component.ts the code patches
"periodPaymentFrequencyType" but the form defines "repaymentFrequencyType";
update the patch to set the value on loansAccountTermsForm using the existing
control name repaymentFrequencyType (e.g., use
loansAccountTermsData.repaymentFrequencyType?.id) or add/rename the form control
accordingly so the template's frequency id is actually written into
loansAccountTermsForm.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d23c130-b208-4ff3-82ba-58ff5c175014
📒 Files selected for processing (9)
src/app/loans/common-resolvers/loan-details.resolver.tssrc/app/loans/common-resolvers/loans-account-and-template.resolver.tssrc/app/loans/edit-loans-account/edit-loans-account.component.tssrc/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.tssrc/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.tssrc/app/loans/loans.service.ts
Comment on lines
32
to
37
| resolve(route: ActivatedRouteSnapshot): Observable<any> { | ||
| const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId'); | ||
| return this.loansService.getLoansAccountAndTemplateResource(loanId); | ||
| return this.loanProductService.isLoanProduct | ||
| ? this.loansService.getLoansAccountAndTemplateResource(loanId) | ||
| : this.loansService.getWorkingCapitalLoanDetails(loanId); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the route configuration to verify resolver ordering
rg -n "LoansAccountAndTemplateResolver" --type ts -A 5 -B 5Repository: openMF/web-app
Length of output: 5266
🏁 Script executed:
#!/bin/bash
# Check the full route configuration for edit-loans-account and surrounding routes
rg -n "edit-loans-account|LoanDetailsResolver" src/app/loans/loans-routing.module.ts -B 3 -A 8Repository: openMF/web-app
Length of output: 5534
🏁 Script executed:
#!/bin/bash
# Check the LoanProductService initialization and isLoanProduct property
rg -n "isLoanProduct|initialize" src/app/loans -B 2 -A 3 --type ts | head -100Repository: openMF/web-app
Length of output: 9749
🏁 Script executed:
#!/bin/bash
# Check if productType is used in edit-loans-account navigation or routes
rg -n "productType|edit-loans-account" src/app/loans --type ts | grep -A 3 -B 3 "edit-loans-account"Repository: openMF/web-app
Length of output: 4331
🏁 Script executed:
#!/bin/bash
# Find LoanProductService class and its properties
fd -e service.ts | xargs grep -l "LoanProductService" | head -5Repository: openMF/web-app
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Search for resolveProductTypeDefault method implementation
rg -n "resolveProductTypeDefault" src/app --type ts -B 2 -A 10Repository: openMF/web-app
Length of output: 6349
🏁 Script executed:
#!/bin/bash
# Check LoanProductService for isLoanProduct property and initialization
rg -n "class LoanProductService|isLoanProduct|initialize\(" src/app/loans -B 2 -A 5 --type ts | grep -A 20 "class LoanProductService"Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Find and read the LoanProductService file
cat -n src/app/products/loan-products/services/loan-product.service.ts | head -150Repository: openMF/web-app
Length of output: 2326
🏁 Script executed:
#!/bin/bash
# Check for isLoanProduct and initialize method in the service
rg -n "isLoanProduct|initialize" src/app/products/loan-products/services/loan-product.service.ts -B 3 -A 5Repository: openMF/web-app
Length of output: 1265
The resolver must initialize the product type from route parameters instead of relying on pre-existing service state.
This resolver depends on loanProductService.isLoanProduct being correctly set, but Angular executes resolvers before the component's constructor. The component initializes the service from query parameters in its constructor (line 78), which runs after the resolver.
If the resolver executes without the productType query parameter being present or the service not yet initialized from a previous page, isLoanProduct will incorrectly default to true, causing the resolver to call getLoansAccountAndTemplateResource() instead of getWorkingCapitalLoanDetails() for working-capital loans.
The resolver should read productType directly from the route's query parameters (as resolveProductTypeDefault does) and call initialize() itself:
const productType = LoanProductBaseComponent.resolveProductTypeDefault(route, 'loan');
this.loanProductService.initialize(productType);
return this.loanProductService.isLoanProduct
? this.loansService.getLoansAccountAndTemplateResource(loanId)
: this.loansService.getWorkingCapitalLoanDetails(loanId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/loans/common-resolvers/loans-account-and-template.resolver.ts` around
lines 32 - 37, The resolver's resolve method must derive productType from the
route instead of relying on existing loanProductService state; call
LoanProductBaseComponent.resolveProductTypeDefault(route, 'loan') to get the
productType, invoke this.loanProductService.initialize(productType), then use
this.loanProductService.isLoanProduct to choose between
this.loansService.getLoansAccountAndTemplateResource(loanId) and
this.loansService.getWorkingCapitalLoanDetails(loanId); update the
resolve(route: ActivatedRouteSnapshot) implementation accordingly.
src/app/loans/edit-loans-account/edit-loans-account.component.ts
Outdated
Show resolved
Hide resolved
...p/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts
Show resolved
Hide resolved
alberto-art3ch
force-pushed
the
WEB-813/working-capital-loan-account-modification
branch
from
35f5839 to
cee719f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/app/loans/edit-loans-account/edit-loans-account.component.ts (1)
102-114:⚠️ Potential issue | 🟠 MajorDon't replace the submitted working-capital account with the generic template payload.
getWorkingCapitalLoansAccountTemplate()is keyed only byclientIdandproductId, sotemplateData.loanDatais template metadata, not this submitted account. ReplacingloansAccountProductTemplatehere drops the edit data loaded fromroute.data—including fields thatsubmitWorkingCapitalProduct()still reads fromclient.id—and moving the option arrays underoptionsalso breaks the top-level shape the stepper still reads.💡 Suggested direction
setTemplate($event: any) { const templateData: any = $event; - this.loansAccountProductTemplate = templateData.loanData ? templateData.loanData : templateData; - this.currencyCode = this.loansAccountProductTemplate.currency.code; - this.productDetails = this.loansAccountProductTemplate.product; if (templateData.loanData) { - this.loansAccountProductTemplate = templateData.loanData; - this.loansAccountProductTemplate.options = { - delinquencyBucketOptions: templateData.delinquencyBucketOptions, - fundOptions: templateData.fundOptions, - periodFrequencyTypeOptions: templateData.periodFrequencyTypeOptions - }; + this.loansAccountProductTemplate = { + ...this.loansAccountProductTemplate, + fundOptions: templateData.fundOptions, + delinquencyBucketOptions: templateData.delinquencyBucketOptions, + periodFrequencyTypeOptions: templateData.periodFrequencyTypeOptions, + options: { + delinquencyBucketOptions: templateData.delinquencyBucketOptions, + fundOptions: templateData.fundOptions, + periodFrequencyTypeOptions: templateData.periodFrequencyTypeOptions + } + }; + } else { + this.loansAccountProductTemplate = templateData; } + this.currencyCode = this.loansAccountProductTemplate.currency.code; + this.productDetails = templateData.product ?? this.loansAccountProductTemplate.product; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/edit-loans-account/edit-loans-account.component.ts` around lines 102 - 114, The setTemplate handler currently overwrites loansAccountProductTemplate with templateData.loanData (from getWorkingCapitalLoansAccountTemplate) which discards the submitted working-capital account loaded from route.data and moves option arrays under an options object; instead, when templateData.loanData exists merge its metadata into the existing loansAccountProductTemplate rather than replacing it: preserve the original top-level shape used by submitWorkingCapitalProduct (so keep client.id and other edit fields), copy or assign delinquencyBucketOptions, fundOptions, periodFrequencyTypeOptions onto the same top-level keys (not nested under options) or merge them into existing arrays, and update currencyCode and productDetails from the resulting merged object; refer to setTemplate, loansAccountProductTemplate, templateData.loanData, submitWorkingCapitalProduct, and the option names to locate and apply the merge logic.src/app/loans/common-resolvers/loans-account-and-template.resolver.ts (1)
18-18:⚠️ Potential issue | 🟠 MajorInitialize the loan product type from the route before branching here.
loanProductService.isLoanProductstill reflects the shared service's previous/default state at this point. On a fresh working-capital edit navigation, this resolver can call the standard/loans/{id}path and hand the component the wrong payload shape.As per coding guidelines, "Use RxJS Observables and route resolvers to fetch and pass data rather than relying on a unified predictable state container".💡 Suggested fix
import { LoanProductService } from 'app/products/loan-products/services/loan-product.service'; +import { LoanProductBaseComponent } from 'app/products/loan-products/common/loan-product-base.component'; ... resolve(route: ActivatedRouteSnapshot): Observable<any> { const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId'); + const productType = LoanProductBaseComponent.resolveProductTypeDefault(route, 'loan'); + this.loanProductService.initialize(productType); return this.loanProductService.isLoanProduct ? this.loansService.getLoansAccountAndTemplateResource(loanId) : this.loansService.getWorkingCapitalLoanDetails(loanId); }Also applies to: 32-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/common-resolvers/loans-account-and-template.resolver.ts` at line 18, The resolver uses loanProductService.isLoanProduct before initializing it from the current route, causing stale/shared state to drive the branching; update the LoansAccountAndTemplateResolver (resolve method) to first derive the loan product type from the incoming ActivatedRouteSnapshot (route params or data) and set/initialize loanProductService (or call a setter/initializer) with that value before any conditional branching that reads loanProductService.isLoanProduct (and similarly for the logic around lines 32-36). Ensure the resolve flow uses the route-derived value (not previous service state) to decide which API/path to call and return the correct observable payload.
🤖 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/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.ts`:
- Around line 124-155: Hydrate the select option arrays when populating an
existing template: ensure that when loansAccountTemplate is set you not only
patch IDs on loansAccountDetailsForm and call
addFormControlsBasedOnProductType(), but also populate fundOptions,
loanOfficerOptions, and loanPurposeOptions from loansAccountTemplate (use the
template's fund/loanOfficer/loanPurpose collections or product metadata) so the
selects have backing options; and make this hydration run on parent updates by
moving the logic into ngOnChanges (or an Input setter for loansAccountTemplate)
and call productChange() as needed to rebuild option lists and productSelected
(referencing loansAccountTemplate, productChange, productSelected,
loanProductsBasicDetails, loanProductService).
---
Duplicate comments:
In `@src/app/loans/common-resolvers/loans-account-and-template.resolver.ts`:
- Line 18: The resolver uses loanProductService.isLoanProduct before
initializing it from the current route, causing stale/shared state to drive the
branching; update the LoansAccountAndTemplateResolver (resolve method) to first
derive the loan product type from the incoming ActivatedRouteSnapshot (route
params or data) and set/initialize loanProductService (or call a
setter/initializer) with that value before any conditional branching that reads
loanProductService.isLoanProduct (and similarly for the logic around lines
32-36). Ensure the resolve flow uses the route-derived value (not previous
service state) to decide which API/path to call and return the correct
observable payload.
In `@src/app/loans/edit-loans-account/edit-loans-account.component.ts`:
- Around line 102-114: The setTemplate handler currently overwrites
loansAccountProductTemplate with templateData.loanData (from
getWorkingCapitalLoansAccountTemplate) which discards the submitted
working-capital account loaded from route.data and moves option arrays under an
options object; instead, when templateData.loanData exists merge its metadata
into the existing loansAccountProductTemplate rather than replacing it: preserve
the original top-level shape used by submitWorkingCapitalProduct (so keep
client.id and other edit fields), copy or assign delinquencyBucketOptions,
fundOptions, periodFrequencyTypeOptions onto the same top-level keys (not nested
under options) or merge them into existing arrays, and update currencyCode and
productDetails from the resulting merged object; refer to setTemplate,
loansAccountProductTemplate, templateData.loanData, submitWorkingCapitalProduct,
and the option names to locate and apply the merge logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e52cdf24-3847-4308-b21f-57c1d8c9ba54
📒 Files selected for processing (9)
src/app/loans/common-resolvers/loan-details.resolver.tssrc/app/loans/common-resolvers/loans-account-and-template.resolver.tssrc/app/loans/edit-loans-account/edit-loans-account.component.tssrc/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.tssrc/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.tssrc/app/loans/loans.service.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts
- src/app/loans/common-resolvers/loan-details.resolver.ts
Comment on lines
124
to
155
| if (this.loansAccountTemplate) { | ||
| this.addFormControlsBasedOnProductType(); | ||
| let loanProductId: number | null = null; | ||
| this.loansAccountDetailsForm.patchValue({ | ||
| fundId: this.loansAccountTemplate.fundId, | ||
| submittedOnDate: | ||
| this.loansAccountTemplate.timeline.submittedOnDate && | ||
| new Date(this.loansAccountTemplate.timeline.submittedOnDate), | ||
| expectedDisbursementDate: | ||
| this.loansAccountTemplate.timeline.expectedDisbursementDate && | ||
| new Date(this.loansAccountTemplate.timeline.expectedDisbursementDate), | ||
| externalId: this.loansAccountTemplate.externalId | ||
| }); | ||
| if (this.loansAccountTemplate.loanProductId) { | ||
| this.addFormControlsBasedOnProductType(); | ||
| loanProductId = this.loansAccountTemplate.loanProductId; | ||
| this.loansAccountDetailsForm.patchValue({ | ||
| productId: this.loansAccountTemplate.loanProductId, | ||
| submittedOnDate: | ||
| this.loansAccountTemplate.timeline.submittedOnDate && | ||
| new Date(this.loansAccountTemplate.timeline.submittedOnDate), | ||
| loanOfficerId: this.loansAccountTemplate.loanOfficerId, | ||
| loanPurposeId: this.loansAccountTemplate.loanPurposeId, | ||
| fundId: this.loansAccountTemplate.fundId, | ||
| expectedDisbursementDate: | ||
| this.loansAccountTemplate.timeline.expectedDisbursementDate && | ||
| new Date(this.loansAccountTemplate.timeline.expectedDisbursementDate), | ||
| externalId: this.loansAccountTemplate.externalId | ||
| loanPurposeId: this.loansAccountTemplate.loanPurposeId | ||
| }); | ||
| this.productSelected = this.loanProductsBasicDetails.find( | ||
| (p: LoanProductBasicDetails) => | ||
| p.productType === this.loanProductService.productType.value && | ||
| p.id === this.loansAccountTemplate.loanProductId | ||
| ); | ||
| if (this.productSelected) { | ||
| this.loansAccountDetailsForm.patchValue({ | ||
| productId: this.productSelected.shortName | ||
| }); | ||
| this.loanProductSelected = true; | ||
| } | ||
| } else if (this.loanProductService.isWorkingCapital) { | ||
| loanProductId = this.loansAccountTemplate.product.id; | ||
| } | ||
| this.productSelected = this.loanProductsBasicDetails.find( | ||
| (p: LoanProductBasicDetails) => | ||
| p.productType === this.loanProductService.productType.value && p.id === loanProductId | ||
| ); | ||
| if (this.productSelected) { | ||
| this.loansAccountDetailsForm.patchValue({ | ||
| productId: this.productSelected.shortName | ||
| }); | ||
| this.loanProductSelected = true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/app/loans/loans-account-stepper/loans-account-details-step && ls -laRepository: openMF/web-app
Length of output: 445
🏁 Script executed:
cat -n src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.ts | head -200Repository: openMF/web-app
Length of output: 8869
🏁 Script executed:
cat -n src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.ts | tail -100Repository: openMF/web-app
Length of output: 4291
🏁 Script executed:
rg "productChange|ngOnChanges|ngOnInit|fundOptions|loanOfficerOptions|loanPurposeOptions" src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.ts -A 5 -B 2Repository: openMF/web-app
Length of output: 1971
🏁 Script executed:
cat -n src/app/loans/edit-loans-account/edit-loans-account.component.ts | grep -A 10 -B 10 "loansAccountTemplate\|working"Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
cat -n src/app/loans/edit-loans-account/edit-loans-account.component.ts | head -150Repository: openMF/web-app
Length of output: 7301
🏁 Script executed:
rg "loansAccountTemplate\|working.*[Cc]apital" src/app/loans/edit-loans-account/edit-loans-account.component.ts -B 3 -A 5Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
grep -n "ngOnChanges\|loansAccountTemplate" src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.tsRepository: openMF/web-app
Length of output: 1138
🏁 Script executed:
rg "loanOfficerOptions|loanPurposeOptions|fundOptions" src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.html -B 2 -A 2Repository: openMF/web-app
Length of output: 943
🏁 Script executed:
rg "fundOptions|loanOfficerOptions|loanPurposeOptions" src/app/loans/loans.service.ts -B 2 -A 10 | head -80Repository: openMF/web-app
Length of output: 559
Populate the select option collections when hydrating an existing account.
The code patches selected IDs (fundId, loanOfficerId, loanPurposeId) from the template at lines 128–142, but fundOptions, loanOfficerOptions, and loanPurposeOptions are never populated. These properties are only assigned inside productChange(), which is not called during the initial edit load. As a result, the select dropdowns will have selected values but no backing options to render their display labels.
Additionally, when the parent component asynchronously fetches and updates the working capital template (via getWorkingCapitalLoanProductTemplate()), the child has no mechanism to react to these changes since it does not implement ngOnChanges() or an input setter for loansAccountTemplate.
Assign the option arrays directly from the template during initialization:
Suggested approach
if (this.loansAccountTemplate) {
this.addFormControlsBasedOnProductType();
+ this.fundOptions = this.loansAccountTemplate.fundOptions ?? [];
+ this.loanOfficerOptions = this.loansAccountTemplate.loanOfficerOptions ?? [];
+ this.loanPurposeOptions = this.loansAccountTemplate.loanPurposeOptions ?? [];
let loanProductId: number | null = null;Move this hydration logic into ngOnChanges() to ensure the options are re-populated whenever the parent updates the template.
📝 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
| if (this.loansAccountTemplate) { | |
| this.addFormControlsBasedOnProductType(); | |
| let loanProductId: number | null = null; | |
| this.loansAccountDetailsForm.patchValue({ | |
| fundId: this.loansAccountTemplate.fundId, | |
| submittedOnDate: | |
| this.loansAccountTemplate.timeline.submittedOnDate && | |
| new Date(this.loansAccountTemplate.timeline.submittedOnDate), | |
| expectedDisbursementDate: | |
| this.loansAccountTemplate.timeline.expectedDisbursementDate && | |
| new Date(this.loansAccountTemplate.timeline.expectedDisbursementDate), | |
| externalId: this.loansAccountTemplate.externalId | |
| }); | |
| if (this.loansAccountTemplate.loanProductId) { | |
| this.addFormControlsBasedOnProductType(); | |
| loanProductId = this.loansAccountTemplate.loanProductId; | |
| this.loansAccountDetailsForm.patchValue({ | |
| productId: this.loansAccountTemplate.loanProductId, | |
| submittedOnDate: | |
| this.loansAccountTemplate.timeline.submittedOnDate && | |
| new Date(this.loansAccountTemplate.timeline.submittedOnDate), | |
| loanOfficerId: this.loansAccountTemplate.loanOfficerId, | |
| loanPurposeId: this.loansAccountTemplate.loanPurposeId, | |
| fundId: this.loansAccountTemplate.fundId, | |
| expectedDisbursementDate: | |
| this.loansAccountTemplate.timeline.expectedDisbursementDate && | |
| new Date(this.loansAccountTemplate.timeline.expectedDisbursementDate), | |
| externalId: this.loansAccountTemplate.externalId | |
| loanPurposeId: this.loansAccountTemplate.loanPurposeId | |
| }); | |
| this.productSelected = this.loanProductsBasicDetails.find( | |
| (p: LoanProductBasicDetails) => | |
| p.productType === this.loanProductService.productType.value && | |
| p.id === this.loansAccountTemplate.loanProductId | |
| ); | |
| if (this.productSelected) { | |
| this.loansAccountDetailsForm.patchValue({ | |
| productId: this.productSelected.shortName | |
| }); | |
| this.loanProductSelected = true; | |
| } | |
| } else if (this.loanProductService.isWorkingCapital) { | |
| loanProductId = this.loansAccountTemplate.product.id; | |
| } | |
| this.productSelected = this.loanProductsBasicDetails.find( | |
| (p: LoanProductBasicDetails) => | |
| p.productType === this.loanProductService.productType.value && p.id === loanProductId | |
| ); | |
| if (this.productSelected) { | |
| this.loansAccountDetailsForm.patchValue({ | |
| productId: this.productSelected.shortName | |
| }); | |
| this.loanProductSelected = true; | |
| } | |
| if (this.loansAccountTemplate) { | |
| this.addFormControlsBasedOnProductType(); | |
| this.fundOptions = this.loansAccountTemplate.fundOptions ?? []; | |
| this.loanOfficerOptions = this.loansAccountTemplate.loanOfficerOptions ?? []; | |
| this.loanPurposeOptions = this.loansAccountTemplate.loanPurposeOptions ?? []; | |
| let loanProductId: number | null = null; | |
| this.loansAccountDetailsForm.patchValue({ | |
| fundId: this.loansAccountTemplate.fundId, | |
| submittedOnDate: | |
| this.loansAccountTemplate.timeline.submittedOnDate && | |
| new Date(this.loansAccountTemplate.timeline.submittedOnDate), | |
| expectedDisbursementDate: | |
| this.loansAccountTemplate.timeline.expectedDisbursementDate && | |
| new Date(this.loansAccountTemplate.timeline.expectedDisbursementDate), | |
| externalId: this.loansAccountTemplate.externalId | |
| }); | |
| if (this.loansAccountTemplate.loanProductId) { | |
| loanProductId = this.loansAccountTemplate.loanProductId; | |
| this.loansAccountDetailsForm.patchValue({ | |
| loanOfficerId: this.loansAccountTemplate.loanOfficerId, | |
| loanPurposeId: this.loansAccountTemplate.loanPurposeId | |
| }); | |
| } else if (this.loanProductService.isWorkingCapital) { | |
| loanProductId = this.loansAccountTemplate.product.id; | |
| } | |
| this.productSelected = this.loanProductsBasicDetails.find( | |
| (p: LoanProductBasicDetails) => | |
| p.productType === this.loanProductService.productType.value && p.id === loanProductId | |
| ); | |
| if (this.productSelected) { | |
| this.loansAccountDetailsForm.patchValue({ | |
| productId: this.productSelected.shortName | |
| }); | |
| this.loanProductSelected = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.ts`
around lines 124 - 155, Hydrate the select option arrays when populating an
existing template: ensure that when loansAccountTemplate is set you not only
patch IDs on loansAccountDetailsForm and call
addFormControlsBasedOnProductType(), but also populate fundOptions,
loanOfficerOptions, and loanPurposeOptions from loansAccountTemplate (use the
template's fund/loanOfficer/loanPurpose collections or product metadata) so the
selects have backing options; and make this hydration run on parent updates by
moving the logic into ngOnChanges (or an Input setter for loansAccountTemplate)
and call productChange() as needed to rebuild option lists and productSelected
(referencing loansAccountTemplate, productChange, productSelected,
loanProductsBasicDetails, loanProductService).
cc33af8 to
b28a59a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/app/loans/edit-loans-account/edit-loans-account.component.ts (1)
102-114:⚠️ Potential issue | 🟠 MajorDon't replace the edit model with the working-capital template response.
getWorkingCapitalLoanProductTemplate()is keyed only byclientIdandproductId, sotemplateData.loanDatacannot uniquely represent the submitted loan being edited. Reassigningthis.loansAccountProductTemplatehere still drops account-scoped fields and makes later reads likesubmitWorkingCapitalProduct()'sclient.iddepend on whatever shape the template endpoint returns.💡 Suggested fix
setTemplate($event: any) { const templateData: any = $event; - this.loansAccountProductTemplate = templateData.loanData ? templateData.loanData : templateData; - this.currencyCode = this.loansAccountProductTemplate.currency.code; - this.productDetails = this.loansAccountProductTemplate.product; if (templateData.loanData) { - this.loansAccountProductTemplate = templateData.loanData; - this.loansAccountProductTemplate.options = { - delinquencyBucketOptions: templateData.delinquencyBucketOptions, - fundOptions: templateData.fundOptions, - periodFrequencyTypeOptions: templateData.periodFrequencyTypeOptions - }; + this.loansAccountProductTemplate = { + ...this.loansAccountProductTemplate, + ...templateData.loanData, + options: { + ...(this.loansAccountProductTemplate?.options ?? {}), + delinquencyBucketOptions: templateData.delinquencyBucketOptions, + fundOptions: templateData.fundOptions, + periodFrequencyTypeOptions: templateData.periodFrequencyTypeOptions + } + }; + } else { + this.loansAccountProductTemplate = templateData; } + this.currencyCode = this.loansAccountProductTemplate.currency.code; + this.productDetails = this.loansAccountProductTemplate.product; }Also applies to: 124-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/edit-loans-account/edit-loans-account.component.ts` around lines 102 - 114, The setTemplate method is overwriting the edit model with the working-capital template response (this.loansAccountProductTemplate = templateData.loanData), which drops account-scoped fields; instead, keep the existing edit model and only merge in template-derived properties (currency, product details, and options) without replacing the whole this.loansAccountProductTemplate; update setTemplate (and the analogous block around lines 124-127) to copy/assign needed fields (e.g., currencyCode, productDetails, and options/delinquencyBucketOptions/fundOptions/periodFrequencyTypeOptions) into the existing this.loansAccountProductTemplate or merge shallowly rather than reassigning the object so client.id and other account-scoped values remain intact.
🧹 Nitpick comments (1)
src/app/core/http/error-handler.interceptor.ts (1)
72-77: The message construction logic is correct.The conditional prefixing of
parameterNameis well-structured. The template literal usage is clean, and the fallback message is appropriately applied.However, per coding guidelines, user-facing strings should use i18n variables. I see this file already has a mix of translated and hardcoded strings (pre-existing pattern), so this is a recommended improvement rather than a blocker.
♻️ Optional: use i18n for the fallback message
const message = parameterName - ? `[${parameterName}] ${errorMessage || 'Invalid parameters were passed in the request!'}` - : `${errorMessage || 'Invalid parameters were passed in the request!'}`; + ? `[${parameterName}] ${errorMessage || this.translate.instant('error.badRequest.invalidParameters')}` + : `${errorMessage || this.translate.instant('error.badRequest.invalidParameters')}`;Then add the corresponding key to your translation files. As per coding guidelines, use proper i18n variables from
@ngx-translate/corefor all user-facing strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/http/error-handler.interceptor.ts` around lines 72 - 77, The current construction of the user-facing fallback string uses a hardcoded literal instead of i18n; update the message formation in error-handler.interceptor.ts (where parameterName and errorMessage are used to build message and passed to this.alertService.alert) to use the translation service (e.g., TranslateService.instant or pipe) with a new translation key for the fallback text, and ensure the parameterName prefix remains applied; also add the new key/value to the project translation files so the fallback message is localized.
🤖 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/core/http/error-handler.interceptor.ts`:
- Around line 52-55: The code is using an incorrect regex and can throw if
defaultUserMessage/developerMessage is undefined; update the replacement to use
the correct regex for periods (/\./g) and guard property access with optional
chaining or a safe fallback before calling replace on errorMessage construction
in the error-handler (refer to errorMessage and
response.error.errors[0].defaultUserMessage / developerMessage), e.g., compute a
source string via optional chaining or nullish coalescing and then call replace
on that string so the fallback is evaluated safely.
- Around line 56-58: The check "'parameterName' in response.error.errors[0]" can
throw when errors[0] is null/undefined; move that membership check inside the
existing null guard that verifies response.error.errors[0] (or replace with a
defensive check like response.error.errors?.[0] && 'parameterName' in
response.error.errors[0]) so you only test for the property after confirming
errors[0] exists; update the logic around the parameterName assignment in the
ErrorHandlerInterceptor (intercept/handleError block where response.error.errors
and parameterName are used) accordingly.
In
`@src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.ts`:
- Around line 150-156: When loading a product template after selecting a product
(inside the productSelected block where loansAccountDetailsForm.patchValue and
getProductTemplate(false) are called), do not overwrite an already-hydrated
fundId from the current loan; change getProductTemplate or its response handler
to detect an existing loansAccountDetailsForm.value.fundId (or an edit-mode
flag) and skip patching response.loanData.fundId into the form if a fundId is
present, and apply the same guard in the other call site referenced (the block
around lines 267-274) so the template fetch never silently replaces the saved
fundId.
In
`@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts`:
- Line 321: The assignment to termFrequencyTypeData lost the top-level fallback;
restore it so termFrequencyTypeData gets periodFrequencyTypeOptions from
loansAccountTermsData.options if present, otherwise from
loansAccountTermsData.periodFrequencyTypeOptions (i.e., use a nullish/||
fallback between loansAccountTermsData.options?.periodFrequencyTypeOptions and
loansAccountTermsData.periodFrequencyTypeOptions) and ensure you handle
loansAccountTermsData being defined before reading its properties.
---
Duplicate comments:
In `@src/app/loans/edit-loans-account/edit-loans-account.component.ts`:
- Around line 102-114: The setTemplate method is overwriting the edit model with
the working-capital template response (this.loansAccountProductTemplate =
templateData.loanData), which drops account-scoped fields; instead, keep the
existing edit model and only merge in template-derived properties (currency,
product details, and options) without replacing the whole
this.loansAccountProductTemplate; update setTemplate (and the analogous block
around lines 124-127) to copy/assign needed fields (e.g., currencyCode,
productDetails, and
options/delinquencyBucketOptions/fundOptions/periodFrequencyTypeOptions) into
the existing this.loansAccountProductTemplate or merge shallowly rather than
reassigning the object so client.id and other account-scoped values remain
intact.
---
Nitpick comments:
In `@src/app/core/http/error-handler.interceptor.ts`:
- Around line 72-77: The current construction of the user-facing fallback string
uses a hardcoded literal instead of i18n; update the message formation in
error-handler.interceptor.ts (where parameterName and errorMessage are used to
build message and passed to this.alertService.alert) to use the translation
service (e.g., TranslateService.instant or pipe) with a new translation key for
the fallback text, and ensure the parameterName prefix remains applied; also add
the new key/value to the project translation files so the fallback message is
localized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f349be1c-3b89-4952-8ad6-ee4861bf29b0
📒 Files selected for processing (23)
src/app/core/http/error-handler.interceptor.tssrc/app/loans/common-resolvers/loan-details.resolver.tssrc/app/loans/common-resolvers/loans-account-and-template.resolver.tssrc/app/loans/edit-loans-account/edit-loans-account.component.tssrc/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.tssrc/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.tssrc/app/loans/loans.service.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 skipped from review due to trivial changes (14)
- src/assets/translations/ko-KO.json
- src/assets/translations/es-CL.json
- src/assets/translations/de-DE.json
- src/assets/translations/es-MX.json
- src/assets/translations/cs-CS.json
- src/assets/translations/fr-FR.json
- src/assets/translations/sw-SW.json
- src/assets/translations/it-IT.json
- src/assets/translations/pt-PT.json
- src/assets/translations/lt-LT.json
- src/assets/translations/en-US.json
- src/assets/translations/lv-LV.json
- src/assets/translations/ne-NE.json
- src/app/loans/loans.service.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/loans/common-resolvers/loan-details.resolver.ts
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html
- src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.html
- src/app/loans/common-resolvers/loans-account-and-template.resolver.ts
Comment on lines
+52
to
+55
| errorMessage = | ||
| response.error.errors[0].defaultUserMessage.replace(/\\./g, ' ') || | ||
| response.error.errors[0].developerMessage.replace(/\\./g, ' '); | ||
| } |
There was a problem hiding this comment.
Regex pattern /\\./g is likely incorrect and property access may throw.
Two issues here:
-
Regex mismatch:
/\\./gmatches a literal backslash followed by any character, not a period. If the intent is to replace periods (.) with spaces, use/\./g. If the intent is to replace backslash-dot sequences (\.), use/\\\./g. -
Potential TypeError: If
defaultUserMessageisundefined, calling.replace()on it will throw before the||fallback is evaluated.
🐛 Proposed fix with optional chaining and corrected regex
Assuming you want to replace periods with spaces:
errorMessage =
- response.error.errors[0].defaultUserMessage.replace(/\\./g, ' ') ||
- response.error.errors[0].developerMessage.replace(/\\./g, ' ');
+ response.error.errors[0].defaultUserMessage?.replace(/\./g, ' ') ||
+ response.error.errors[0].developerMessage?.replace(/\./g, ' ');Or if you want to replace backslash-dot (\.) sequences with spaces:
errorMessage =
- response.error.errors[0].defaultUserMessage.replace(/\\./g, ' ') ||
- response.error.errors[0].developerMessage.replace(/\\./g, ' ');
+ response.error.errors[0].defaultUserMessage?.replace(/\\\./g, ' ') ||
+ response.error.errors[0].developerMessage?.replace(/\\\./g, ' ');📝 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
| errorMessage = | |
| response.error.errors[0].defaultUserMessage.replace(/\\./g, ' ') || | |
| response.error.errors[0].developerMessage.replace(/\\./g, ' '); | |
| } | |
| errorMessage = | |
| response.error.errors[0].defaultUserMessage?.replace(/\./g, ' ') || | |
| response.error.errors[0].developerMessage?.replace(/\./g, ' '); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/core/http/error-handler.interceptor.ts` around lines 52 - 55, The
code is using an incorrect regex and can throw if
defaultUserMessage/developerMessage is undefined; update the replacement to use
the correct regex for periods (/\./g) and guard property access with optional
chaining or a safe fallback before calling replace on errorMessage construction
in the error-handler (refer to errorMessage and
response.error.errors[0].defaultUserMessage / developerMessage), e.g., compute a
source string via optional chaining or nullish coalescing and then call replace
on that string so the fallback is evaluated safely.
Comment on lines
+56
to
58
| if ('parameterName' in response.error.errors[0]) { | ||
| parameterName = response.error.errors[0].parameterName; | ||
| } |
There was a problem hiding this comment.
Bug: parameterName check is outside the null guard and will throw if errors[0] is falsy.
The 'parameterName' in response.error.errors[0] check is placed outside the if (response.error.errors[0]) block. If response.error.errors exists but errors[0] is undefined or null, this will throw a TypeError because the in operator cannot be used on non-objects.
🐛 Proposed fix: move the check inside the existing null guard
if (response.error.errors) {
if (response.error.errors[0]) {
errorMessage =
response.error.errors[0].defaultUserMessage.replace(/\\./g, ' ') ||
response.error.errors[0].developerMessage.replace(/\\./g, ' ');
- }
- if ('parameterName' in response.error.errors[0]) {
- parameterName = response.error.errors[0].parameterName;
+ if ('parameterName' in response.error.errors[0]) {
+ parameterName = response.error.errors[0].parameterName;
+ }
}
}📝 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
| if ('parameterName' in response.error.errors[0]) { | |
| parameterName = response.error.errors[0].parameterName; | |
| } | |
| if (response.error.errors) { | |
| if (response.error.errors[0]) { | |
| errorMessage = | |
| response.error.errors[0].defaultUserMessage.replace(/\\./g, ' ') || | |
| response.error.errors[0].developerMessage.replace(/\\./g, ' '); | |
| if ('parameterName' in response.error.errors[0]) { | |
| parameterName = response.error.errors[0].parameterName; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/core/http/error-handler.interceptor.ts` around lines 56 - 58, The
check "'parameterName' in response.error.errors[0]" can throw when errors[0] is
null/undefined; move that membership check inside the existing null guard that
verifies response.error.errors[0] (or replace with a defensive check like
response.error.errors?.[0] && 'parameterName' in response.error.errors[0]) so
you only test for the property after confirming errors[0] exists; update the
logic around the parameterName assignment in the ErrorHandlerInterceptor
(intercept/handleError block where response.error.errors and parameterName are
used) accordingly.
...ans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.ts
Show resolved
Hide resolved
| this.loansAccountTermsForm.patchValue({ | ||
| principalAmount: this.loansAccountTermsData.product.principal | ||
| }); | ||
| this.termFrequencyTypeData = this.loansAccountTermsData.options?.periodFrequencyTypeOptions; |
There was a problem hiding this comment.
Keep the top-level fallback for periodFrequencyTypeOptions.
Line 321 now reads only options?.periodFrequencyTypeOptions. This step still receives working-capital template data in more than one shape, so when options is absent the repayment-frequency select ends up with no options.
💡 Suggested fix
- this.termFrequencyTypeData = this.loansAccountTermsData.options?.periodFrequencyTypeOptions;
+ this.termFrequencyTypeData =
+ this.loansAccountTermsData.options?.periodFrequencyTypeOptions ??
+ this.loansAccountTermsData.periodFrequencyTypeOptions;📝 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
| this.termFrequencyTypeData = this.loansAccountTermsData.options?.periodFrequencyTypeOptions; | |
| this.termFrequencyTypeData = | |
| this.loansAccountTermsData.options?.periodFrequencyTypeOptions ?? | |
| this.loansAccountTermsData.periodFrequencyTypeOptions; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts`
at line 321, The assignment to termFrequencyTypeData lost the top-level
fallback; restore it so termFrequencyTypeData gets periodFrequencyTypeOptions
from loansAccountTermsData.options if present, otherwise from
loansAccountTermsData.periodFrequencyTypeOptions (i.e., use a nullish/||
fallback between loansAccountTermsData.options?.periodFrequencyTypeOptions and
loansAccountTermsData.periodFrequencyTypeOptions) and ensure you handle
loansAccountTermsData being defined before reading its properties.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
321-321:⚠️ Potential issue | 🟠 MajorKeep the top-level fallback for repayment frequency options.
This step still receives working-capital data in more than one shape. When
optionsis absent,termFrequencyTypeDatabecomesundefinedand the frequency select loses its choices.💡 Suggested fix
- this.termFrequencyTypeData = this.loansAccountTermsData.options?.periodFrequencyTypeOptions; + this.termFrequencyTypeData = + this.loansAccountTermsData.options?.periodFrequencyTypeOptions ?? + this.loansAccountTermsData.periodFrequencyTypeOptions;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts` at line 321, The assignment to termFrequencyTypeData uses only loansAccountTermsData.options.periodFrequencyTypeOptions, which becomes undefined when options is missing and causes the frequency select to lose choices; modify the logic in the component (where termFrequencyTypeData is set) to fall back to a top-level repayment frequency source on loansAccountTermsData (e.g., check loansAccountTermsData.periodFrequencyTypeOptions or a known top-level key) when options is absent, so termFrequencyTypeData is populated from loansAccountTermsData.options?.periodFrequencyTypeOptions || loansAccountTermsData.periodFrequencyTypeOptions (or the equivalent top-level property) to ensure the select retains choices.src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.ts (1)
267-274:⚠️ Potential issue | 🟠 MajorDon't overwrite the saved
fundIdwith the template default.This template call only knows
clientId+productId, soresponse.loanData.fundIdis not guaranteed to be the submitted account's current fund. Patching it here can silently change the edit form before submit.💡 Suggested fix
- this.loansAccountDetailsForm.patchValue({ - fundId: response.loanData.fundId - }); + if (emitEvent || !this.loansAccountDetailsForm.get('fundId')?.value) { + this.loansAccountDetailsForm.patchValue({ + fundId: response.loanData.fundId + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.ts` around lines 267 - 274, The code calls loansService.getWorkingCapitalLoansAccountTemplate and then unconditionally patches loansAccountDetailsForm with response.loanData.fundId, which can overwrite the saved account's fund; remove that unconditional patch or guard it so it only sets fundId when the form has no existing value (e.g., if loansAccountDetailsForm.get('fundId').value is null/undefined). Keep emitting loansAccountProductTemplate and setting fundOptions as-is, but do not overwrite the user's current fund selection from the template response.loanData.fundId unless the form is empty.src/app/loans/edit-loans-account/edit-loans-account.component.ts (1)
102-114:⚠️ Potential issue | 🟠 MajorNormalize the working-capital template without replacing the edit model.
getWorkingCapitalLoansAccountTemplate()only fetches template metadata forclientId+productId, so its response is not the submitted loan record. ReplacingloansAccountProductTemplatehere changes the shape for downstream consumers — for example, the preview still readsloansAccountProductTemplate.fundOptions, but this branch only keepsfundOptionsunderoptions.💡 Suggested fix
setTemplate($event: any) { const templateData: any = $event; - this.loansAccountProductTemplate = templateData.loanData ? templateData.loanData : templateData; + this.loansAccountProductTemplate = templateData.loanData + ? { ...this.loansAccountProductTemplate, ...templateData.loanData } + : templateData; this.currencyCode = this.loansAccountProductTemplate.currency.code; this.productDetails = this.loansAccountProductTemplate.product; if (templateData.loanData) { - this.loansAccountProductTemplate = templateData.loanData; - this.loansAccountProductTemplate.options = { - delinquencyBucketOptions: templateData.delinquencyBucketOptions, - fundOptions: templateData.fundOptions, - periodFrequencyTypeOptions: templateData.periodFrequencyTypeOptions - }; + this.loansAccountProductTemplate = { + ...this.loansAccountProductTemplate, + delinquencyBucketOptions: templateData.delinquencyBucketOptions, + fundOptions: templateData.fundOptions, + periodFrequencyTypeOptions: templateData.periodFrequencyTypeOptions, + options: { + delinquencyBucketOptions: templateData.delinquencyBucketOptions, + fundOptions: templateData.fundOptions, + periodFrequencyTypeOptions: templateData.periodFrequencyTypeOptions + } + }; }Also applies to: 124-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/edit-loans-account/edit-loans-account.component.ts` around lines 102 - 114, The current setTemplate replaces the edit model (this.loansAccountProductTemplate) with template metadata when templateData.loanData is present, which changes object shape and breaks downstream consumers (e.g., preview expecting fundOptions at top level); instead, in setTemplate (and the same fix at the other branch around lines 124-126) merge/normalize the metadata onto the existing edit model rather than overwriting it: keep the original this.loansAccountProductTemplate object (the submitted loan record), update its top-level fields used elsewhere (currencyCode, productDetails) and copy templateData.delinquencyBucketOptions, fundOptions, periodFrequencyTypeOptions both into an options sub-object and, if other code expects them at top-level, also assign them to the matching top-level properties (e.g., fundOptions) so previews and consumers continue to find the expected keys. Ensure you only assign templateData.loanData fields to metadata fields and do not replace the whole loansAccountProductTemplate object.
🤖 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/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts`:
- Around line 324-330: The patchValue call in
loans-account-terms-step.component.ts currently sets discount using
`this.loansAccountTermsData.discount || ''`, which converts a valid 0 to an
empty string; change it to preserve zero by using a nullish check (e.g., use
`??` or an explicit undefined/null check) when setting discount in the
loansAccountTermsForm.patchValue so that 0 remains 0 while undefined/null still
become the empty fallback; update the reference in the same patchValue block
where discount is assigned (loansAccountTermsData.discount ->
loansAccountTermsForm.patchValue).
- Line 49: The import for amountValueValidator in
loans-account-terms-step.component.ts is unresolved because
app/shared/validators/amount-value.validator does not exist; fix by either
adding a new validator file that exports amountValueValidator() in the
validators directory or updating the component to use an existing validator
(e.g., minNumberValueValidator or maxNumberValueValidator / percentageValidator)
exported from min-number-value.validator.ts, max-number-value.validator.ts or
percentage.validator.ts; ensure the component calls the correct exported
function name (amountValueValidator or the chosen existing validator) where
amountValueValidator() is used in LoansAccountTermsStepComponent so TypeScript
error TS2307 is resolved.
---
Duplicate comments:
In `@src/app/loans/edit-loans-account/edit-loans-account.component.ts`:
- Around line 102-114: The current setTemplate replaces the edit model
(this.loansAccountProductTemplate) with template metadata when
templateData.loanData is present, which changes object shape and breaks
downstream consumers (e.g., preview expecting fundOptions at top level);
instead, in setTemplate (and the same fix at the other branch around lines
124-126) merge/normalize the metadata onto the existing edit model rather than
overwriting it: keep the original this.loansAccountProductTemplate object (the
submitted loan record), update its top-level fields used elsewhere
(currencyCode, productDetails) and copy templateData.delinquencyBucketOptions,
fundOptions, periodFrequencyTypeOptions both into an options sub-object and, if
other code expects them at top-level, also assign them to the matching top-level
properties (e.g., fundOptions) so previews and consumers continue to find the
expected keys. Ensure you only assign templateData.loanData fields to metadata
fields and do not replace the whole loansAccountProductTemplate object.
In
`@src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.ts`:
- Around line 267-274: The code calls
loansService.getWorkingCapitalLoansAccountTemplate and then unconditionally
patches loansAccountDetailsForm with response.loanData.fundId, which can
overwrite the saved account's fund; remove that unconditional patch or guard it
so it only sets fundId when the form has no existing value (e.g., if
loansAccountDetailsForm.get('fundId').value is null/undefined). Keep emitting
loansAccountProductTemplate and setting fundOptions as-is, but do not overwrite
the user's current fund selection from the template response.loanData.fundId
unless the form is empty.
In
`@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts`:
- Line 321: The assignment to termFrequencyTypeData uses only
loansAccountTermsData.options.periodFrequencyTypeOptions, which becomes
undefined when options is missing and causes the frequency select to lose
choices; modify the logic in the component (where termFrequencyTypeData is set)
to fall back to a top-level repayment frequency source on loansAccountTermsData
(e.g., check loansAccountTermsData.periodFrequencyTypeOptions or a known
top-level key) when options is absent, so termFrequencyTypeData is populated
from loansAccountTermsData.options?.periodFrequencyTypeOptions ||
loansAccountTermsData.periodFrequencyTypeOptions (or the equivalent top-level
property) to ensure the select retains choices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03c0d0ae-d40d-4680-98db-5023a6ec2e11
📒 Files selected for processing (23)
src/app/core/http/error-handler.interceptor.tssrc/app/loans/common-resolvers/loan-details.resolver.tssrc/app/loans/common-resolvers/loans-account-and-template.resolver.tssrc/app/loans/edit-loans-account/edit-loans-account.component.tssrc/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.tssrc/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.tssrc/app/loans/loans.service.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 skipped from review due to trivial changes (11)
- src/assets/translations/es-CL.json
- src/assets/translations/it-IT.json
- src/assets/translations/en-US.json
- src/assets/translations/ko-KO.json
- src/assets/translations/cs-CS.json
- src/assets/translations/sw-SW.json
- src/assets/translations/ne-NE.json
- src/assets/translations/fr-FR.json
- src/assets/translations/es-MX.json
- src/assets/translations/lt-LT.json
- src/assets/translations/lv-LV.json
🚧 Files skipped from review as they are similar to previous changes (6)
- src/app/loans/common-resolvers/loans-account-and-template.resolver.ts
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html
- src/app/loans/common-resolvers/loan-details.resolver.ts
- src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.html
- src/assets/translations/de-DE.json
- src/assets/translations/pt-PT.json
...p/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts
Show resolved
Hide resolved
Comment on lines
+324
to
+330
| this.loansAccountTermsForm.patchValue({ | ||
| discount: this.loansAccountTermsData.discount || '', | ||
| principalAmount: this.loansAccountTermsData.proposedPrincipal, | ||
| periodPaymentRate: this.loansAccountTermsData.periodPaymentRate, | ||
| totalPayment: this.loansAccountTermsData.balance?.totalPayment, | ||
| repaymentEvery: this.loansAccountTermsData.repaymentEvery, | ||
| repaymentFrequencyType: this.loansAccountTermsData.repaymentFrequencyType?.id |
There was a problem hiding this comment.
Preserve zero-valued discounts when patching edit data.
|| '' turns a legitimate 0 into an empty string. That hides the saved value in the form and can submit discount: '' if the user edits other fields.
💡 Suggested fix
- discount: this.loansAccountTermsData.discount || '',
+ discount: this.loansAccountTermsData.discount ?? '',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts`
around lines 324 - 330, The patchValue call in
loans-account-terms-step.component.ts currently sets discount using
`this.loansAccountTermsData.discount || ''`, which converts a valid 0 to an
empty string; change it to preserve zero by using a nullish check (e.g., use
`??` or an explicit undefined/null check) when setting discount in the
loansAccountTermsForm.patchValue so that 0 remains 0 while undefined/null still
become the empty fallback; update the reference in the same patchValue block
where discount is assigned (loansAccountTermsData.discount ->
loansAccountTermsForm.patchValue).
b28a59a to
c418fa8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
599-626:⚠️ Potential issue | 🟡 MinorValidator additions depend on missing
amountValueValidator.The new validation rules using
amountValueValidator()at lines 601, 608, and 625 will not work until the missing validator file is created. Once implemented, ensureamountValueValidatorproperly validates positive numeric amounts and doesn't conflict withValidators.min(1)onrepaymentEvery.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts` around lines 599 - 626, Create and export an amountValueValidator function (used by totalPayment, amount and repaymentEvery form controls) that ensures values are positive numeric amounts (non-negative/greater-than-zero as appropriate) and returns the standard Angular ValidatorFn signature; place it where other shared validators live and import it into loans-account-terms-step.component.ts so calls to amountValueValidator() resolve. Ensure the validator's semantics align with existing Validators.min(1): for controls that already use Validators.min(1) (repaymentEvery, periodPaymentRate) either remove the redundant min check or have amountValueValidator accept a configurable minimum (e.g., min=1) so it doesn't conflict; update usages accordingly and add unit tests for positive number handling and interaction with Validators.min(1).
🧹 Nitpick comments (3)
src/app/directives/positive-number.directive.ts (1)
15-27: Directive only blocks arrow-key decrement; other negative-value entry paths remain open.This directive prevents
ArrowDownfrom going below zero, but does not block:
- Typing
-followed by digits- Pasting negative values
- Programmatic
setValue()/patchValue()calls on the form controlPer the relevant code snippet in
loans-account-terms-step.component.ts(lines 595-632),totalPaymentanddiscountform controls lackValidators.min(), andamountValueValidator()only checks format—not minimum value. The HTMLmin="0"attribute provides browser-level hint but not form-level validation.Consider adding a
Validators.min(0)to those form controls for comprehensive protection.♻️ Extend directive to also block direct negative input
`@HostListener`('keydown', ['$event']) onKeyDown(event: KeyboardEvent): void { const inputElement = this.el.nativeElement; + // Block typing '-' character when at start of input + if (event.key === '-' || event.key === 'Minus') { + event.preventDefault(); + return; + } + // Check if the key is ArrowUp or ArrowDown if (event.key === 'ArrowUp' || event.key === 'ArrowDown') { const currentValue = parseFloat(inputElement.value) || 0; if (event.key === 'ArrowDown' && currentValue <= 0) { // Prevent default if trying to decrement below or at 0 event.preventDefault(); } } } + +@HostListener('paste', ['$event']) +onPaste(event: ClipboardEvent): void { + const pastedData = event.clipboardData?.getData('text') || ''; + if (parseFloat(pastedData) < 0) { + event.preventDefault(); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/directives/positive-number.directive.ts` around lines 15 - 27, The directive currently only blocks ArrowDown; update positive-number.directive.ts by adding HostListeners for 'paste' and 'input' (e.g., add onPaste(event: ClipboardEvent) and onInput(event: Event)) to sanitize/persistently reject negative signs and pasted negative values (preventDefault on paste if clipboard starts with '-' and strip leading '-' on input), and keep the existing onKeyDown logic for Arrow keys; separately, in loans-account-terms-step.component.ts add Validators.min(0) to the totalPayment and discount FormControl declarations (alongside amountValueValidator()) so programmatic setValue()/patchValue() and form validation also enforce non-negative values.src/app/shared/validators/amount-value.validator.ts (1)
16-22: Misleading inline comment does not match the actual regex.The comment describes a lookahead pattern
(?=.{1,14}$)that is not present in the actual regex on line 22. The implemented regex^\d{1,13}(\.\d{1,6})?$correctly validates up to 13 integer digits with an optional decimal part of up to 6 digits—but the comment may confuse future maintainers.📝 Suggested fix
- // Regex breakdown: - // ^ - Start of string - // (?=.{1,14}$) - Lookahead to ensure total length (including potential dot) - // [0-9]+ - One or more digits - // (\.[0-9]{1,6})? - Optional dot followed by 1 to 6 digits - // $ - End of string + // Regex breakdown: + // ^ - Start of string + // \d{1,13} - 1 to 13 integer digits (maxTotalDigits - maxDecimals) + // (\.\d{1,6})? - Optional decimal point followed by 1 to 6 digits + // $ - End of string const regex = new RegExp(`^\\d{1,${maxTotalDigits - maxDecimals}}(\\.\\d{1,${maxDecimals}})?$`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/validators/amount-value.validator.ts` around lines 16 - 22, The inline comment above the const regex is misleading (mentions a lookahead that doesn't exist); update the comment next to the regex constant to accurately describe the implemented pattern for regex (const regex) and the role of maxTotalDigits and maxDecimals — e.g., state that it enforces up to (maxTotalDigits - maxDecimals) integer digits followed by an optional decimal point with up to maxDecimals digits, matching the actual pattern ^\\d{1,${maxTotalDigits - maxDecimals}}(\\.\\d{1,${maxDecimals}})?$ so future maintainers understand the numeric limits.src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.ts (1)
147-153: Add null safety for working capital fund options path.When
isLoanProductis false,this.loansAccountProductTemplate.options?.fundOptionsmay be undefined if theoptionsobject is missing, causingfundOptions.find(...)to throw. Consider adding optional chaining or a fallback.💡 Suggested fix
fundName(fundId: number): string { const fundOptions = this.loanProductService.isLoanProduct ? this.loansAccountProductTemplate.fundOptions - : this.loansAccountProductTemplate.options.fundOptions; - const fund = fundOptions.find((fund: any) => fund.id === fundId); + : this.loansAccountProductTemplate.options?.fundOptions; + const fund = fundOptions?.find((f: any) => f.id === fundId); return fund ? fund.name : ''; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.ts` around lines 147 - 153, The fundName method can throw if loansAccountProductTemplate.options is undefined; update it to defensively read fundOptions by using optional chaining and a default empty array when isLoanProduct is false (e.g., use this.loansAccountProductTemplate.options?.fundOptions ?? []) so find() is called on a safe array, and keep the existing return logic in fundName(fundId: number).
🤖 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/directives/positive-number.directive.ts`:
- Around line 11-12: The PositiveNumberDirective is missing the standalone: true
flag in its `@Directive` metadata causing the DirectivesModule import to fail;
update the `@Directive` decorator for PositiveNumberDirective to include
standalone: true (i.e., add standalone: true inside the decorator object) so the
directive can be imported directly in module imports.
In `@src/app/loans/common-resolvers/loan-delinquency-actions.resolver.ts`:
- Around line 29-33: The resolver's resolve method should validate loanId before
calling the service: mirror LoanDelinquencyTagsResolver's check (e.g., ensure
loanId is present and numeric with !isNaN(+loanId)); if invalid, return an
observable using of(null) (or the same fallback used by the tags resolver)
instead of calling loansService.getDelinquencyActions; add import { of } from
'rxjs' at the top and update the resolve function in
LoanDelinquencyActionsResolver to perform this validation before invoking
getDelinquencyActions.
In `@src/app/loans/common-resolvers/loan-delinquency-data.resolver.ts`:
- Around line 34-42: The resolve method can return undefined when loanId is
missing/non-numeric or when isLoanProduct is false; change
resolve(ActivatedRouteSnapshot) so it always returns an Observable (use of(...)
from 'rxjs') as a safe fallback instead of falling through to undefined, e.g.,
return of({ loanDelinquencyData: { delinquent: false, currency: '' } }) when
!isLoanProduct or loanId is invalid; ensure you import { of } from 'rxjs' and
keep the existing branch that returns
this.loansService.getDelinquencyData(loanId) when isLoanProduct and loanId is
valid.
In `@src/app/loans/common-resolvers/loan-delinquency-tags.resolver.ts`:
- Around line 35-41: The resolver resolve(route: ActivatedRouteSnapshot) can
return undefined when loanId is non-numeric; update it to always return an
Observable by importing of from 'rxjs' and returning a safe fallback Observable
when loanId is invalid (e.g., of(null) or of({ loanDelinquencyData: {
delinquent: false, currency: null } })), keeping the successful path that calls
this.loansService.getDelinquencyTags(this.loanAccountPath, loanId) unchanged;
reference resolve, loanId, loanAccountPath and loansService.getDelinquencyTags
when making the change.
---
Duplicate comments:
In
`@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts`:
- Around line 599-626: Create and export an amountValueValidator function (used
by totalPayment, amount and repaymentEvery form controls) that ensures values
are positive numeric amounts (non-negative/greater-than-zero as appropriate) and
returns the standard Angular ValidatorFn signature; place it where other shared
validators live and import it into loans-account-terms-step.component.ts so
calls to amountValueValidator() resolve. Ensure the validator's semantics align
with existing Validators.min(1): for controls that already use Validators.min(1)
(repaymentEvery, periodPaymentRate) either remove the redundant min check or
have amountValueValidator accept a configurable minimum (e.g., min=1) so it
doesn't conflict; update usages accordingly and add unit tests for positive
number handling and interaction with Validators.min(1).
---
Nitpick comments:
In `@src/app/directives/positive-number.directive.ts`:
- Around line 15-27: The directive currently only blocks ArrowDown; update
positive-number.directive.ts by adding HostListeners for 'paste' and 'input'
(e.g., add onPaste(event: ClipboardEvent) and onInput(event: Event)) to
sanitize/persistently reject negative signs and pasted negative values
(preventDefault on paste if clipboard starts with '-' and strip leading '-' on
input), and keep the existing onKeyDown logic for Arrow keys; separately, in
loans-account-terms-step.component.ts add Validators.min(0) to the totalPayment
and discount FormControl declarations (alongside amountValueValidator()) so
programmatic setValue()/patchValue() and form validation also enforce
non-negative values.
In
`@src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.ts`:
- Around line 147-153: The fundName method can throw if
loansAccountProductTemplate.options is undefined; update it to defensively read
fundOptions by using optional chaining and a default empty array when
isLoanProduct is false (e.g., use
this.loansAccountProductTemplate.options?.fundOptions ?? []) so find() is called
on a safe array, and keep the existing return logic in fundName(fundId: number).
In `@src/app/shared/validators/amount-value.validator.ts`:
- Around line 16-22: The inline comment above the const regex is misleading
(mentions a lookahead that doesn't exist); update the comment next to the regex
constant to accurately describe the implemented pattern for regex (const regex)
and the role of maxTotalDigits and maxDecimals — e.g., state that it enforces up
to (maxTotalDigits - maxDecimals) integer digits followed by an optional decimal
point with up to maxDecimals digits, matching the actual pattern
^\\d{1,${maxTotalDigits - maxDecimals}}(\\.\\d{1,${maxDecimals}})?$ so future
maintainers understand the numeric limits.
🪄 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: 9cb29d05-f009-4926-b35b-401ed79528a1
📒 Files selected for processing (32)
src/app/core/http/error-handler.interceptor.tssrc/app/directives/directives.module.tssrc/app/directives/positive-number.directive.tssrc/app/loans/common-resolvers/loan-base.resolver.tssrc/app/loans/common-resolvers/loan-delinquency-actions.resolver.tssrc/app/loans/common-resolvers/loan-delinquency-data.resolver.tssrc/app/loans/common-resolvers/loan-delinquency-tags.resolver.tssrc/app/loans/common-resolvers/loan-details.resolver.tssrc/app/loans/common-resolvers/loans-account-and-template.resolver.tssrc/app/loans/edit-loans-account/edit-loans-account.component.tssrc/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.tssrc/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.tssrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.tssrc/app/loans/loans-view/loan-delinquency-tags-tab/loan-delinquency-tags-tab.component.tssrc/app/loans/loans.service.tssrc/app/shared/validators/amount-value.validator.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 skipped from review due to trivial changes (13)
- src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.html
- src/assets/translations/es-CL.json
- src/assets/translations/es-MX.json
- src/assets/translations/sw-SW.json
- src/assets/translations/lt-LT.json
- src/assets/translations/it-IT.json
- src/assets/translations/de-DE.json
- src/assets/translations/lv-LV.json
- src/assets/translations/ne-NE.json
- src/assets/translations/en-US.json
- src/assets/translations/cs-CS.json
- src/assets/translations/ko-KO.json
- src/assets/translations/fr-FR.json
🚧 Files skipped from review as they are similar to previous changes (7)
- src/app/loans/common-resolvers/loans-account-and-template.resolver.ts
- src/app/loans/common-resolvers/loan-details.resolver.ts
- src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html
- src/assets/translations/pt-PT.json
- src/app/core/http/error-handler.interceptor.ts
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html
- src/app/loans/loans-account-stepper/loans-account-details-step/loans-account-details-step.component.ts
Comment on lines
+11
to
+12
| @Directive({ selector: '[mifosxPositiveNumber]' }) | ||
| export class PositiveNumberDirective { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other directives in the same module are standalone
fd -e ts -p 'directive' src/app/directives --exec grep -l "standalone.*true" {} \;
# Check the exact pattern used by existing directives
rg -A2 "@Directive" src/app/directives/Repository: openMF/web-app
Length of output: 1313
🏁 Script executed:
cat -n src/app/directives/directives.module.tsRepository: openMF/web-app
Length of output: 1445
Missing standalone: true will cause module import to fail.
The DirectivesModule imports this directive directly into its imports array, but the directive is not marked as standalone. This will cause a compilation error since Angular 14+ requires all classes in an imports array to be standalone.
🐛 Proposed fix
-@Directive({ selector: '[mifosxPositiveNumber]' })
+@Directive({ selector: '[mifosxPositiveNumber]', standalone: true })
export class PositiveNumberDirective {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/directives/positive-number.directive.ts` around lines 11 - 12, The
PositiveNumberDirective is missing the standalone: true flag in its `@Directive`
metadata causing the DirectivesModule import to fail; update the `@Directive`
decorator for PositiveNumberDirective to include standalone: true (i.e., add
standalone: true inside the decorator object) so the directive can be imported
directly in module imports.
Comment on lines
29
to
33
| resolve(route: ActivatedRouteSnapshot): Observable<any> { | ||
| this.initialize(route); | ||
| const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId'); | ||
| return this.loansService.getDelinquencyActions(loanId); | ||
| return this.loansService.getDelinquencyActions(this.loanAccountPath, loanId); | ||
| } |
There was a problem hiding this comment.
Consider adding loanId validation for consistency.
Unlike LoanDelinquencyTagsResolver (which checks !isNaN(+loanId)), this resolver unconditionally calls the service. If loanId is null or non-numeric, the request will be made to an invalid endpoint (e.g., /{productType}/null/delinquency-actions).
💡 Suggested fix for consistency
resolve(route: ActivatedRouteSnapshot): Observable<any> {
this.initialize(route);
const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId');
+ if (!loanId || isNaN(+loanId)) {
+ return of([]);
+ }
return this.loansService.getDelinquencyActions(this.loanAccountPath, loanId);
}Add import { of } from 'rxjs'; at the top.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/loans/common-resolvers/loan-delinquency-actions.resolver.ts` around
lines 29 - 33, The resolver's resolve method should validate loanId before
calling the service: mirror LoanDelinquencyTagsResolver's check (e.g., ensure
loanId is present and numeric with !isNaN(+loanId)); if invalid, return an
observable using of(null) (or the same fallback used by the tags resolver)
instead of calling loansService.getDelinquencyActions; add import { of } from
'rxjs' at the top and update the resolve function in
LoanDelinquencyActionsResolver to perform this validation before invoking
getDelinquencyActions.
Comment on lines
34
to
42
| resolve(route: ActivatedRouteSnapshot): Observable<any> { | ||
| this.initialize(route); | ||
| const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId'); | ||
| return this.loansService.getDelinquencyData(loanId); | ||
| if (!isNaN(+loanId)) { | ||
| if (this.isLoanProduct) { | ||
| return this.loansService.getDelinquencyData(loanId); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Return a safe fallback to prevent downstream NPE.
When isLoanProduct is false (working-capital) or loanId is non-numeric, resolve() returns undefined. The consumer component accesses data.loanDelinquencyData.delinquent and data.loanDelinquencyData.currency without null-checking, causing a runtime error.
🛠️ Suggested fix
resolve(route: ActivatedRouteSnapshot): Observable<any> {
this.initialize(route);
const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId');
if (!isNaN(+loanId)) {
if (this.isLoanProduct) {
return this.loansService.getDelinquencyData(loanId);
}
}
+ // Return empty object to prevent NPE in consumers
+ return of({});
}Add import { of } from 'rxjs'; at the top.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/loans/common-resolvers/loan-delinquency-data.resolver.ts` around
lines 34 - 42, The resolve method can return undefined when loanId is
missing/non-numeric or when isLoanProduct is false; change
resolve(ActivatedRouteSnapshot) so it always returns an Observable (use of(...)
from 'rxjs') as a safe fallback instead of falling through to undefined, e.g.,
return of({ loanDelinquencyData: { delinquent: false, currency: '' } }) when
!isLoanProduct or loanId is invalid; ensure you import { of } from 'rxjs' and
keep the existing branch that returns
this.loansService.getDelinquencyData(loanId) when isLoanProduct and loanId is
valid.
Comment on lines
35
to
41
| resolve(route: ActivatedRouteSnapshot): Observable<any> { | ||
| this.initialize(route); | ||
| const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId'); | ||
| return this.loansService.getDelinquencyTags(loanId); | ||
| if (!isNaN(+loanId)) { | ||
| return this.loansService.getDelinquencyTags(this.loanAccountPath, loanId); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing return value may cause downstream NPE.
When loanId is non-numeric, resolve() returns undefined. The consumer component (loan-delinquency-tags-tab.component.ts lines 121-122) accesses data.loanDelinquencyData.delinquent and data.loanDelinquencyData.currency without null-checking, which will throw a runtime error.
Consider returning an empty observable or a safe fallback object:
🛠️ Suggested fix
resolve(route: ActivatedRouteSnapshot): Observable<any> {
this.initialize(route);
const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId');
if (!isNaN(+loanId)) {
return this.loansService.getDelinquencyTags(this.loanAccountPath, loanId);
}
+ return of([]);
}Add import { of } from 'rxjs'; at the top.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/loans/common-resolvers/loan-delinquency-tags.resolver.ts` around
lines 35 - 41, The resolver resolve(route: ActivatedRouteSnapshot) can return
undefined when loanId is non-numeric; update it to always return an Observable
by importing of from 'rxjs' and returning a safe fallback Observable when loanId
is invalid (e.g., of(null) or of({ loanDelinquencyData: { delinquent: false,
currency: null } })), keeping the successful path that calls
this.loansService.getDelinquencyTags(this.loanAccountPath, loanId) unchanged;
reference resolve, loanId, loanAccountPath and loansService.getDelinquencyTags
when making the change.
alberto-art3ch
force-pushed
the
WEB-813/working-capital-loan-account-modification
branch
from
c418fa8 to
d6b07d7
Compare
This was referenced Mar 31, 2026
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
User should be able to modify a submitted Loan account of working capital loan product View is considering the fields that getting via template
Related issues and discussion
WEB-813
Screenshots
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
Improvements
Localization