WEB-979: Loan view broken for non Working Capital#3645
Open
alberto-art3ch wants to merge 1 commit into
Open
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Base resolver truthiness guard src/app/loans/common-resolvers/loan-base.resolver.ts |
LoanBaseResolver.initialize() changes the productType check from a null-specific (!== null) to a truthiness guard, preventing empty-string values from entering product-type mapping logic. |
Service method path parameterization src/app/loans/loans.service.ts |
getLoansAccountCharge() signature updated to accept loanAccountPath first and construct the HTTP path using /${loanAccountPath}/${accountId}/charges/${chargeId}; getWorkingCapitalPeriodPaymentRates now returns Observable<PeriodPaymentRateChange[]> and imports the model. |
Account-charge resolver inheritance and validation src/app/loans/common-resolvers/loans-account-charge.resolver.ts |
LoansAccountChargeResolver now extends LoanBaseResolver, calls initialize(route), extracts loanId/chargeId from route params, validates loanId is numeric, invokes getLoansAccountCharge(this.loanAccountPath, loanId, chargeId) on valid ids, and returns of([]) when loanId is not numeric. |
Working capital period-rates resolver src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts |
LoanPeriodPaymentRatesResolver extends LoanBaseResolver, calls initialize(route), validates numeric loanId and this.isWorkingCapital before calling getWorkingCapitalPeriodPaymentRates(loanId); otherwise returns of([]). Return type narrowed to Observable<PeriodPaymentRateChange[]>. |
View component defensive null-safety src/app/loans/loans-view/loans-view.component.html, src/app/loans/loans-view/loans-view.component.ts |
Template and component updated to use optional chaining: transactions?.length and product?.name to avoid runtime errors when those properties are missing. |
sequenceDiagram
participant LoansAccountChargeResolver
participant LoansService
participant API
LoansAccountChargeResolver->>LoansService: getLoansAccountCharge(loanAccountPath, loanId, chargeId)
LoansService->>API: GET /{loanAccountPath}/{accountId}/charges/{chargeId}
API-->>LoansService: HTTP response
LoansService-->>LoansAccountChargeResolver: Observable<...>
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly Related PRs
- openMF/web-app#3435: Both PRs involve refactoring loan resolvers to use
LoanBaseResolverfor initialization and path routing.
Suggested Reviewers
- adamsaghy
- IOhacker
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'WEB-979: Loan view broken for non Working Capital' accurately reflects the main objective of the pull request, which addresses a bug where the loan view was broken/incomplete for non-working-capital loan types. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
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: 4
🧹 Nitpick comments (1)
src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts (1)
31-32: 💤 Low valueValidate loanId existence before numeric check.
Line 31 extracts
loanIdwhich could benull. Line 32's numeric check!isNaN(+loanId)convertsnullto0, and!isNaN(0)returnstrue, allowing the code to proceed with an invalid ID. While the service call would likely fail, it's clearer to validate existence first.♻️ Proposed refactor for clarity
resolve(route: ActivatedRouteSnapshot): Observable<any> | null { this.initialize(route); const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId'); - if (!isNaN(+loanId)) { + if (loanId && !isNaN(+loanId)) { if (this.isWorkingCapital) { return this.loansService.getWorkingCapitalPeriodPaymentRates(loanId); } } return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts` around lines 31 - 32, The code currently casts route.paramMap.get('loanId') (which can be null) to a number directly; change the check in the resolver (loan-period-payment-rates.resolver.ts) to first verify loanId is non-null/defined and then validate it's numeric before proceeding. Locate the loanId extraction (const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId')) and replace the guard with an existence check (e.g., loanId != null or !== null) combined with the numeric check (e.g., !isNaN(+loanId)) so you only call the service when loanId is present and numeric.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/loans/common-resolvers/loans-account-charge.resolver.ts`:
- Around line 32-39: The resolver resolve method (resolve in
loans-account-charge.resolver.ts) can return undefined when +loanId is NaN;
change the method signature from Observable<any> to Observable<any> | null and
add an explicit "return null" for the invalid loanId branch (i.e., after the
isNaN(+loanId) check) so the function always returns the declared type; keep the
successful path using
this.loansService.getLoansAccountCharge(this.loanAccountPath, loanId, chargeId).
In `@src/app/loans/loans-view/loans-view.component.html`:
- Line 360: Resolver payload may omit transactions for non-working-capital loans
causing TransactionsTabComponent to fail; update LoanDetailsResolver (the code
that calls loansService.getLoanAccountAssociationDetails(...) and
getWorkingCapitalLoanDetails(...)) to always include transactions: [] in the
returned loanDetailsData, or alternatively add a defensive fallback in
TransactionsTabComponent so that this.loanDetailsData.transactions is set to an
empty array if undefined before any iteration; reference LoanDetailsResolver,
loansService.getLoanAccountAssociationDetails, getWorkingCapitalLoanDetails, and
TransactionsTabComponent when making the change.
In
`@src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts`:
- Line 29: The resolver method resolve(route: ActivatedRouteSnapshot) currently
returns Observable<any>; replace any with a concrete type by defining or
importing a PeriodPaymentRateChange interface that models the API response,
update the resolver signature to return Observable<PeriodPaymentRateChange> |
null, and update the corresponding service method (e.g., the method in
LoansService that fetches period payment rates) to return
Observable<PeriodPaymentRateChange> instead of Observable<any>; ensure all
usages and imports are adjusted to the new type so TypeScript strict mode is
satisfied.
- Around line 29-37: The resolver's resolve method can return null which breaks
the consuming component expecting PeriodPaymentRateChange[]; modify resolve (in
loan-period-payment-rates.resolver.ts) to always return an array by importing
and using RxJS of(...) and return of([]) when no valid loanId or when not
isWorkingCapital, while keeping the existing branch that returns
this.loansService.getWorkingCapitalPeriodPaymentRates(loanId); ensure the import
for of is added to the file.
---
Nitpick comments:
In
`@src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts`:
- Around line 31-32: The code currently casts route.paramMap.get('loanId')
(which can be null) to a number directly; change the check in the resolver
(loan-period-payment-rates.resolver.ts) to first verify loanId is
non-null/defined and then validate it's numeric before proceeding. Locate the
loanId extraction (const loanId = route.paramMap.get('loanId') ||
route.parent.paramMap.get('loanId')) and replace the guard with an existence
check (e.g., loanId != null or !== null) combined with the numeric check (e.g.,
!isNaN(+loanId)) so you only call the service when loanId is present and
numeric.
🪄 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: a0ed0c94-e761-4a9d-a775-49d0f9566ad9
📒 Files selected for processing (6)
src/app/loans/common-resolvers/loan-base.resolver.tssrc/app/loans/common-resolvers/loans-account-charge.resolver.tssrc/app/loans/loans-view/loans-view.component.htmlsrc/app/loans/loans-view/loans-view.component.tssrc/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.tssrc/app/loans/loans.service.ts
alberto-art3ch
force-pushed
the
WEB-979/loans-view-broken-for-non-working-capital
branch
from
196fcf5 to
279fbdb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts`:
- Around line 26-29: The JSDoc for the resolver method currently declares
"`@returns` {Observable<any>}" but the method actually returns
Observable<PeriodPaymentRateChange[]>; update the JSDoc on the method in
loan-period-payment-rates.resolver.ts to use "`@returns`
{Observable<PeriodPaymentRateChange[]>}" and adjust the short description to
reflect it returns period payment rate change objects (e.g., "Returns period
payment rate change data. `@returns` {Observable<PeriodPaymentRateChange[]>}").
- Around line 31-37: The code is allowing null loanId because +null === 0;
change the validation in the resolver to first ensure the retrieved loanId
string exists (non-null/undefined) and then validate it's numeric before calling
the service: get the id into a local variable (e.g., loanIdStr from
route.paramMap.get(...) || route.parent.paramMap.get(...)), check if (loanIdStr
&& !isNaN(Number(loanIdStr))) and only then call
this.loansService.getWorkingCapitalPeriodPaymentRates(loanIdStr); update
references in initialize(route) / loan-period-payment-rates.resolver functions
accordingly.
🪄 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: 81a87be6-e7a7-48a9-90e1-53a6d62ae32a
📒 Files selected for processing (6)
src/app/loans/common-resolvers/loan-base.resolver.tssrc/app/loans/common-resolvers/loans-account-charge.resolver.tssrc/app/loans/loans-view/loans-view.component.htmlsrc/app/loans/loans-view/loans-view.component.tssrc/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.tssrc/app/loans/loans.service.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/app/loans/common-resolvers/loan-base.resolver.ts
- src/app/loans/loans-view/loans-view.component.html
- src/app/loans/loans-view/loans-view.component.ts
- src/app/loans/loans.service.ts
- src/app/loans/common-resolvers/loans-account-charge.resolver.ts
alberto-art3ch
force-pushed
the
WEB-979/loans-view-broken-for-non-working-capital
branch
from
279fbdb to
e08719a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/loans/loans.service.ts (1)
879-879: 💤 Low valueConsider tightening the
loanIdparameter type.The
loanIdparameter is typed asany, while similar methods in this service usestring(e.g.,getLoanChargeson line 599,getLoanChargeTemplateResourceon line 37). For consistency and type safety, consider changing it tostring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans.service.ts` at line 879, The parameter type for getWorkingCapitalPeriodPaymentRates is currently any; change the loanId parameter to string to match other methods (e.g., getLoanCharges, getLoanChargeTemplateResource) for consistency and stronger type safety, update the function signature getWorkingCapitalPeriodPaymentRates(loanId: string): Observable<PeriodPaymentRateChange[]> and any internal usages or callers that pass or expect a different type so they align with the new string type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/loans/loans.service.ts`:
- Line 879: The parameter type for getWorkingCapitalPeriodPaymentRates is
currently any; change the loanId parameter to string to match other methods
(e.g., getLoanCharges, getLoanChargeTemplateResource) for consistency and
stronger type safety, update the function signature
getWorkingCapitalPeriodPaymentRates(loanId: string):
Observable<PeriodPaymentRateChange[]> and any internal usages or callers that
pass or expect a different type so they align with the new string type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fcc7d3ea-579c-4385-857a-1c6ca552bae3
📒 Files selected for processing (6)
src/app/loans/common-resolvers/loan-base.resolver.tssrc/app/loans/common-resolvers/loans-account-charge.resolver.tssrc/app/loans/loans-view/loans-view.component.htmlsrc/app/loans/loans-view/loans-view.component.tssrc/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.tssrc/app/loans/loans.service.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/loans/loans-view/loans-view.component.html
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/loans/loans-view/loans-view.component.ts
- src/app/loans/common-resolvers/loans-account-charge.resolver.ts
- src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
UI loans view appears Broken Incomplete for non Working Capital
Related issues and discussion
WEB-979
Screenshots, if any
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
Bug Fixes
New Features
Refactor