Aman-Mittal commented on PR #524:
URL:
https://github.com/apache/fineract-backoffice-ui/pull/524#issuecomment-5560583110
> Used AI to review this
>
> Reviewed locally against `55abe539`. The diagnosis is right, the fix is
the right shape, and the tests are real — I reverted just the component and
re-ran your spec:
>
> ```
> 5 failed | 5 passed (10) AssertionError: expected true to be false
> ```
>
> and all 10 pass with it. Deriving `isEditMode` after the id has been
validated is exactly the right move: the original bug was that `isEditMode` and
the two `if (!this.variationId)` guards were computed from the same segment by
two different truthiness rules, and that can no longer happen.
>
> To your question — redirecting is the right call, and it matches what #512
suggested. Staying put on an `/edit/` URL that is really a create form is the
worse of the two. If you want to go one better, a brief notification on the way
out would tell the user why they moved, but I would not hold the PR for it.
>
> Two things I would like changed, and one nit.
> ### 1. The redirect can land on a route that does not exist
>
> `onCancel()` navigates to `['/loans', this.loanId, 'interest-pauses']`.
Now that `loanId` also goes through `toRouteId`,
`/loans/abc/interest-pauses/edit/7` sets `loanId` to `null` and the redirect
becomes `/loans/null/interest-pauses`. Confirmed by asserting on the spy:
>
> ```
> expected [ [ '/loans', null, 'interest-pauses' ] ]
> ```
>
> This is not a regression — before your change that same path produced
`/loans/NaN/interest-pauses` — but your PR is the natural place to finish it,
since `toRouteId` is now telling you the id is unusable. Falling back to
`/loans` when `loanId` is null would close it.
> ### 2. An unusable loan id still leaves a dead Edit screen
>
> With `loanId` unusable but `variationId` fine
(`/loans/abc/interest-pauses/edit/7`), the component sets `isEditMode` to true,
`loadPause()` returns early on `!this.loanId`, and `onSubmit` returns early on
`if (!this.loanId) return;`. The result is a screen that says Edit Interest
Pause, shows nothing, and does nothing at all when Save is pressed — the same
shape as the bug you are fixing, one field over.
>
> `toRouteId(loanId) === null` is already the signal; sending that case to
`/loans` alongside case 1 handles both.
> ### 3. Nit: `%p` does not substitute here
>
> All five parameterized cases render their title literally, so when one
fails you cannot tell which id it was:
>
> ```
> × should not enter edit mode for the unusable variation id %p
> × should not enter edit mode for the unusable variation id %p
> ...
> ```
>
> `%s` does substitute — verified by forcing a failure:
>
> ```
> × should not enter edit mode for the unusable variation id abc
> × should not enter edit mode for the unusable variation id 0
> × should not enter edit mode for the unusable variation id -1
> × should not enter edit mode for the unusable variation id 1.5
> ```
>
> Worth having, since the five cases are the point of the table.
> ### About those "pre-existing failures"
>
> They are not pre-existing — your `node_modules` has drifted from
`package-lock.json`. I lost a while to exactly this today and diagnosed it the
wrong way round at first. Check with:
>
> ```
> node -e
"console.log(require('./node_modules/@angular/build/package.json').version,
require('./node_modules/vitest/package.json').version)"
> ```
>
> If that prints anything other than `22.1.0 4.1.11`, `npm ci` will fix it.
On the older builder, files that pass individually fail when another file is in
the same run, which is what makes it look environmental.
>
> Your branch on correct dependencies:
>
> ```
> Test Files 236 passed (236)
> Tests 1424 passed (1424)
> ```
>
> So the suite is clean and `interest-pause-form` is not a special case in
it.
>
> Happy to approve once 1 and 2 are in. Thanks for picking this up — the
parameterized table covering `0`, `-1` and `1.5` alongside `abc` is more
thorough than the issue asked for.
Merging This we can handle the changes in separate PR
--
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]
