Aman-Mittal commented on code in PR #799:
URL:
https://github.com/apache/fineract-backoffice-ui/pull/799#discussion_r4302020142
##########
e2e/client-empty-account-cta.spec.ts:
##########
@@ -88,6 +88,33 @@ async function mockClientWithoutAccounts(page: Page):
Promise<void> {
});
}
+test('loan account rows show the principal returned by the accounts endpoint',
async ({ page }) => {
Review Comment:
Minor:
- **Where the test lives.** This spec is about empty-account CTAs; a test of
a populated loan row will be hard to find here. Consider a
`client-accounts.spec.ts` (or the existing client spec), sharing `signIn` and
the client mocks.
- **Mocked vs real backend.** This mock encodes the same assumption as the
fix (`originalLoan`), so it can't catch the next contract drift. The bug only
appeared against a real response. If the real-backend suite already seeds a
client with a loan, one assertion there that the Loan Accounts principal is
non-zero and matches the loan's principal would guard the actual contract. If
not, the mocked test is fine to keep.
##########
src/app/features/clients/client-view.component.ts:
##########
@@ -765,7 +765,7 @@ export type ClientTab = (typeof CLIENT_TAB)[keyof typeof
CLIENT_TAB];
</th>
<td cdk-cell *cdkCellDef="let account">
{{ account.currency?.displaySymbol }}
- {{ account.originalPrincipal || 0 | number: '1.2-2'
}}
+ {{ account.originalLoan || 0 | number: '1.2-2' }}
Review Comment:
**Please drop the `|| 0` fallback.** #742 pointed out that it turned a
missing field into a believable `$ 0.00`, which is why the wrong binding went
unnoticed. With it still here, the next contract drift (or a loan without
`originalLoan`) will hide the same way. On a financial figure, "unknown" should
look unknown:
```html
@if (account.originalLoan != null) {
{{ account.currency?.displaySymbol }} {{ account.originalLoan | number:
'1.2-2' }}
} @else {
—
}
```
The `!= null` check keeps a real `0` rendering as `$ 0.00`, which your test
already covers. Please add a third fixture row with no `originalLoan`,
asserting the placeholder.
Separately, `account` is untyped in the cdk cell, and
`GetClientsLoanAccounts` doesn't declare `originalLoan` (that's why the test
needs `as any`). A local type in the style of #768, `GetClientsLoanAccounts & {
originalLoan?: number; loanBalance?: number; amountPaid?: number }` on the
`loanAccounts` signal, documents the gap until the model is regenerated. The
template still won't type-check through `let account`, but the fixtures and any
TS access will.
Optional, as #742 suggested: a `loanBalance` (outstanding) column, which is
usually what an officer wants on this tab.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
