Aman-Mittal commented on code in PR #768:
URL:
https://github.com/apache/fineract-backoffice-ui/pull/768#discussion_r4292518742
##########
src/app/features/products/loan-product-form.component.ts:
##########
@@ -1216,7 +1221,7 @@ const DAILY_INTEREST_CALCULATION_PERIOD = 0;
>
@for (option of repaymentStartDateTypeOptions(); track
option.id) {
<ion-select-option [value]="option.id">{{
- option.description
+ option.value ?? option.description
Review Comment:
This is right for this select. The same `option.description` binding is
still used by five other template-backed selects in this file: compounding type
(~1000), reschedule strategy (~1027), recalculation frequency (~1054 and ~1106)
and pre-closure strategy (~1160). The live template returns `value` for all of
them.
Rather than repeating `option.value ?? option.description` six times, a
small helper would keep them consistent and avoid an empty row if both are
missing:
```ts
/** Fineract enum options carry their label in `value`; generated types
still say `description` (#653). */
protected enumLabel(option: { value?: string; description?: string; code?:
string }): string {
return option.value ?? option.description ?? option.code ?? '';
}
```
```html
<ion-select-option [value]="option.id">{{ enumLabel(option)
}}</ion-select-option>
```
If you add translation (see the review summary), this is also the single
place to look up a code-keyed i18n entry first.
##########
src/app/features/products/loan-product-form.component.ts:
##########
@@ -87,6 +87,11 @@ import { TooltipDirective } from
'../../shared/directives/tooltip.directive';
*/
const DAILY_INTEREST_CALCULATION_PERIOD = 0;
+/** The live template response uses `value`; the generated contract currently
omits it. */
+type RepaymentStartDateTypeOption = GetLoanProductsRepaymentStartDateType & {
Review Comment:
The local widening is a reasonable workaround while the generated model is
wrong. Two small suggestions:
1. Make it generic, so the other five option signals can use it too, and
link the tracking issue so it's removed once the generated contract is fixed:
```ts
/**
* Fineract serialises enum options as `{ id, code, value }`
(BaseEnumOptionData). The generated
* `GetLoanProducts*Type` models declare `description` instead — see #653.
Drop once regenerated.
*/
type TemplateEnumOption<T> = T & { value?: string };
```
2. The doc comment says the generated contract "omits" `value`. More
precisely, it declares a `description` that the server never sends. Worth
saying so, so nobody "fixes" this by switching back to `description`.
##########
src/app/features/products/loan-product-form.component.test.ts:
##########
@@ -113,7 +113,16 @@ const TEMPLATE = {
},
],
repaymentStartDateTypeOptions: [
Review Comment:
Good catch correcting this fixture to the real shape and codes. The
neighbouring fixtures in the same `TEMPLATE` still have the old, unreal shape,
which is why the other blank dropdowns aren't caught:
- `interestRecalculationCompoundingTypeOptions` (line ~89) uses
`description: 'None'`. The live response is
`{"id":0,"code":"interestRecalculationCompoundingMethod.none","value":"None"}`.
- The same applies to `interestRecalculationFrequencyTypeOptions`,
`rescheduleStrategyTypeOptions` and
`preClosureInterestCalculationStrategyOptions`.
- The pre-closure code has also drifted: the fixture has
`preClosureInterestCalculationStrategy.tillPreClosureDate`, but the live code
is `loanPreClosureInterestCalculationStrategy.tillPreClosureDate`.
Could you move those to `value` too? Together with the helper suggested on
the template, a test in the style of the one you added would then cover all six
selects, with interest recalculation enabled so the section renders.
##########
src/app/features/products/loan-product-form.component.test.ts:
##########
@@ -492,7 +501,18 @@ describe('LoanProductFormComponent', () => {
expect(component.capitalizedIncomeTypeOptions()).toHaveLength(2);
expect(component.capitalizedIncomeStrategyOptions()[0].value).toBe(EQUAL_AMORTIZATION_LABEL);
expect(component.chargeOffBehaviourOptions()).toHaveLength(2);
- expect(component.repaymentStartDateTypeOptions()).toHaveLength(1);
+ expect(component.repaymentStartDateTypeOptions()).toHaveLength(2);
+ });
+
+ it('renders repayment start date labels from the template response', () =>
{
+ const select = fixture.nativeElement.querySelector(
+ '[data-testid="loan-product-repayment-start-date-type"]',
+ );
+ const labels =
Array.from<HTMLElement>(select.querySelectorAll('ion-select-option')).map(
+ (option) => option.textContent.trim(),
+ );
+
+ expect(labels).toEqual(['Disbursement Date', 'Submitted On Date']);
Review Comment:
Nice: asserting the rendered option text, rather than just the signal
length, is what actually catches this class of bug. Two optional additions:
- A case where an option has neither `value` nor `description`, asserting a
non-empty label (the `code`, or a translated key). That locks in "never render
a blank option".
- If translation lands, assert the translated text for one non-English
locale, or at least that the label comes through the translate pipe.
--
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]
