fix(loans): send an unusable interest-pause id back to the list - #524
Merged
Aman-Mittal merged 1 commit intoSep 6, 2026
Merged
Conversation
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Contributor
E2E — mocked backend🎭 E2E Tests✅ All green — 342 passed · 0 failed · 0 skipped, across 28 spec files in 6m 44s. By spec file
All 342 tests — click to expand
Slowest 10 — what the shard counts should be tuned against
📼 Download the HTML report, videos and traces — see the Generated by run 34032778412 from |
Contributor
E2E — real Fineract🎭 E2E Tests✅ All green — 77 passed · 0 failed · 0 skipped · 1 flaky, across 23 spec files in 5m 56s.
|
| Spec | ✅ | ❌ | ⏭️ | Time |
|---|---|---|---|---|
| backend.setup.ts | 3 | 0 | 0 | 4.7s |
| batch-api-operations.spec.ts | 5 | 0 | 0 | 26.4s |
| center-servicing.spec.ts | 2 | 0 | 0 | 15.2s |
| client-transfer.spec.ts | 4 | 0 | 0 | 40.6s |
| deposit-account-servicing.spec.ts | 3 | 0 | 0 | 22.9s |
| deposit-product-configuration.spec.ts | 3 | 0 | 0 | 13.4s |
| group-membership.spec.ts | 3 | 0 | 0 | 41.8s |
| loan-account-actions.spec.ts | 3 | 0 | 0 | 19.0s |
| loan-charge-off.spec.ts | 2 | 0 | 0 | 16.5s |
| loan-product-accounting.spec.ts | 1 | 0 | 0 | 29.2s |
| loan-servicing.spec.ts | 2 | 0 | 0 | 15.2s |
| login.spec.ts | 4 | 0 | 0 | 5.5s |
| parity-screens.spec.ts | 8 | 0 | 0 | 39.2s |
| rbac-backend-restricted-user.spec.ts | 7 | 0 | 0 | 32.1s |
| rbac-multi-permission.spec.ts | 9 | 0 | 0 | 52.9s |
| full-demo.spec.ts | 1 | 0 | 0 | 54.5s |
| loan-lifecycle.spec.ts | 4 | 0 | 0 | 1m 47s |
| loan-schedule-type.spec.ts | 3 | 0 | 0 | 29.4s |
| report-parameter-backend.spec.ts | 4 | 0 | 0 | 19.6s |
| savings-transaction-correction.spec.ts | 1 | 0 | 0 | 6.1s |
| share-account-servicing.spec.ts | 2 | 0 | 0 | 16.8s |
| share-product-accounting.spec.ts | 1 | 0 | 0 | 21.1s |
| teller-cash-management.spec.ts | 2 | 0 | 0 | 32.4s |
All 77 tests — click to expand
backend.setup.ts
- ✅ seed backend reference data —
1.5s - ✅ seed backend reference data —
1.6s - ✅ seed backend reference data —
1.5s
batch-api-operations.spec.ts
- ✅ Batch API Operations against Fineract › runs the sample batch scenario — create client, create loan, add and read back a charge —
4.9s - ✅ Batch API Operations against Fineract › shows a parse error instead of submitting when the batch input is not valid JSON —
5.4s - ✅ Batch API enclosingTransaction semantics against Fineract › rolls back the earlier steps when enclosingTransaction is true and a later step fails —
4.8s - ✅ Batch API enclosingTransaction semantics against Fineract › does not roll back the earlier steps when enclosingTransaction is false and a later step fails —
3.7s - ✅ Batch API Operations on a mobile viewport against Fineract › the sample batch scenario is reachable and works by touch at mobile width —
7.5s(retried 1×)
center-servicing.spec.ts
- ✅ Center servicing › a center is activated, staffed and given a group —
11.2s - ✅ Center servicing › notes are recorded against the center —
4.0s
client-transfer.spec.ts
- ✅ Client transfer between offices › a proposed transfer is held until the destination accepts, and then the client moves —
10.3s - ✅ Client transfer between offices › a rejected transfer leaves the client on hold, and withdrawing is the way back —
10.8s - ✅ Client transfer between offices › a client can be transferred in one step when the user may act for both offices —
9.4s - ✅ Client staff assignment › an officer can be assigned and then removed —
10.2s
deposit-account-servicing.spec.ts
- ✅ Term deposit account servicing › an account is approved, activated and closed before maturity —
9.9s - ✅ Term deposit account servicing › a deposit is recorded, listed, and reversed without leaving the list —
8.1s - ✅ Term deposit account servicing › an application can be rejected instead of approved —
4.8s
deposit-product-configuration.spec.ts
- ✅ Deposit product configuration › a fixed deposit product survives being edited —
4.7s - ✅ Deposit product configuration › a recurring deposit product can be created at all —
3.8s - ✅ Deposit product configuration › a savings product carries its accounting configuration —
4.9s
group-membership.spec.ts
- ✅ Group membership and lifecycle › a group is activated, staffed, given members and a committee, then emptied —
18.2s - ✅ Group membership and lifecycle › notes are recorded against the group and can be removed again —
6.7s - ✅ Group membership and lifecycle › an empty group is closed with a reason, and a group with members is refused —
16.9s
loan-account-actions.spec.ts
- ✅ Loan account lifecycle actions › new action menu items appear only for active loans —
5.7s - ✅ Loan account lifecycle actions › undo disbursal shows a confirm dialog and reverts the loan to Approved —
6.5s - ✅ Loan account lifecycle actions › write off requires confirmation and moves the loan out of Active status —
6.7s
loan-charge-off.spec.ts
- ✅ Loan servicing commands › charges a loan off through the UI and reverses it —
8.7s - ✅ Loan servicing commands › records a goodwill credit through the shared transaction form —
7.8s
loan-product-accounting.spec.ts
- ✅ Loan product accounting › a cash-accounting product is configured, round-trips on edit, and posts to the ledger —
29.2s
loan-servicing.spec.ts
- ✅ Loan servicing: notes and transaction adjustment › notes can be added and removed, with a confirm dialog on delete —
7.2s - ✅ Loan servicing: notes and transaction adjustment › a repayment transaction can be viewed and adjusted with a corrected amount —
8.1s
login.spec.ts
- ✅ Login › login page displays correctly —
1.2s - ✅ Login › login form has required fields —
1.3s - ✅ Login › submit button is disabled when form is empty —
1.7s - ✅ Login › submit button is enabled when form is filled —
1.3s
parity-screens.spec.ts
- ✅ Screens added for platform parity › a manual journal entry can be read whole and reversed —
4.4s - ✅ Screens added for platform parity › an entry that is already reversed is not offered again —
4.7s - ✅ Screens added for platform parity › a report definition can be created, edited and deleted; a core one cannot —
4.4s - ✅ Screens added for platform parity › a core report opens read-only with only its in-use setting —
4.4s - ✅ Screens added for platform parity › a pending loan is approved from the queue, in a batch —
5.5s - ✅ Screens added for platform parity › a fixed deposit is listed as a deposit, not as a savings account —
5.3s - ✅ Screens added for platform parity › an office has a screen, and it carries its custom fields —
4.1s - ✅ Screens added for platform parity › a savings account carries notes, and the note survives a reload —
6.3s
rbac-backend-restricted-user.spec.ts
- ✅ a genuinely restricted Fineract user › holds exactly the permissions their role was granted —
212ms - ✅ a genuinely restricted Fineract user › reaches the screen their permission covers —
4.0s - ✅ a genuinely restricted Fineract user › is refused a screen their permission does not cover, by URL and by the backend —
5.0s - ✅ a genuinely restricted Fineract user › is refused a write screen they can read the list for, and the write itself —
5.7s - ✅ a genuinely restricted Fineract user › is not offered the actions it would be refused for —
3.7s - ✅ a genuinely restricted Fineract user › is shown an action it cannot take, disabled and saying what it needs —
5.5s - ✅ a genuinely restricted Fineract user › the superuser the rest of the suite uses is unaffected —
8.0s
rbac-multi-permission.spec.ts
- ✅ a route declaring more than one permission code (OR semantics) › is admitted by either declared code alone —
4.7s - ✅ a route declaring more than one permission code (OR semantics) › is admitted by the other declared code alone —
3.9s - ✅ a route declaring more than one permission code (OR semantics) › is refused when holding neither declared code, by the router and by the backend —
4.8s - ✅ ALL_FUNCTIONS_READ, against the real Fineract permission catalogue › reaches read screens across modules it holds no specific code for —
8.6s - ✅ ALL_FUNCTIONS_READ, against the real Fineract permission catalogue › is refused every write screen, and the writes themselves —
6.2s - ✅ a restricted session across a real page reload › keeps the same permission boundary after reloading, not just after a fresh login —
8.8s - ✅ a second real action-level gate, distinct from loan repayment › is shown the Approve action disabled and naming what it needs, refused by the backend too —
5.9s - ✅ Security module writes (users, roles), against the real backend › reaches the list screens but is refused the write screens —
9.5s - ✅ Security module writes (users, roles), against the real backend › is refused creating a user and modifying a role, by the backend itself —
291ms
full-demo.spec.ts
- ✅ Full feature demo recording › walk through loan schedule type, lifecycle, custom fields, collateral, and disbursement —
54.5s
loan-lifecycle.spec.ts
- ✅ Loan lifecycle: creation, approval, disbursement › create, approve, and disburse a Cumulative loan —
28.3s - ✅ Loan lifecycle: creation, approval, disbursement › create, approve, and disburse a Progressive loan —
28.3s - ✅ Loan lifecycle: creation, approval, disbursement › an approved loan can be returned to pending approval —
25.9s - ✅ Loan lifecycle: creation, approval, disbursement › the delinquency tab reads a real loan, and the empty data tabs stay hidden —
24.3s
loan-schedule-type.spec.ts
- ✅ Loan Schedule Type (Cumulative vs Progressive) › loan products list shows a schedule type chip per product —
7.5s - ✅ Loan Schedule Type (Cumulative vs Progressive) › create a Progressive loan product end-to-end and verify it round-trips —
13.1s - ✅ Loan Schedule Type (Cumulative vs Progressive) › loan creation shows the schedule type badge for a Progressive product —
8.8s
report-parameter-backend.spec.ts
- ✅ Dynamic report parameters against Fineract › keeps the parameter form available when a report has cascading lookups —
4.0s - ✅ Dynamic report parameters against Fineract › changing Office changes the Client Listing row set —
5.4s - ✅ Cascading report parameters against Fineract › sends the parent value to the child lookup and clears the child when it changes —
6.5s - ✅ Chart reports against Fineract › renders a chart report as a chart rather than a table —
3.6s
savings-transaction-correction.spec.ts
- ✅ Savings transaction correction › a deposit is reversed and a hold is released —
6.1s
share-account-servicing.spec.ts
- ✅ Share account servicing › an account is approved, activated, traded and closed —
11.6s - ✅ Share account servicing › an application can be rejected —
5.2s
share-product-accounting.spec.ts
- ✅ Share product accounting › a share product is mapped to equity and round-trips on edit —
21.1s
teller-cash-management.spec.ts
- ✅ Teller cash management › a cashier is listed, receives an allocation, and settles cash back —
22.3s - ✅ Teller cash management › settling more than the cashier holds is refused and the form stays usable —
10.1s
Slowest 10 — what the shard counts should be tuned against
| Test | Spec | Time |
|---|---|---|
| Full feature demo recording › walk through loan schedule type, lifecycle, custom fields, collateral, and disbursement | full-demo.spec.ts |
54.5s |
| Loan product accounting › a cash-accounting product is configured, round-trips on edit, and posts to the ledger | loan-product-accounting.spec.ts |
29.2s |
| Loan lifecycle: creation, approval, disbursement › create, approve, and disburse a Cumulative loan | loan-lifecycle.spec.ts |
28.3s |
| Loan lifecycle: creation, approval, disbursement › create, approve, and disburse a Progressive loan | loan-lifecycle.spec.ts |
28.3s |
| Loan lifecycle: creation, approval, disbursement › an approved loan can be returned to pending approval | loan-lifecycle.spec.ts |
25.9s |
| Loan lifecycle: creation, approval, disbursement › the delinquency tab reads a real loan, and the empty data tabs stay hidden | loan-lifecycle.spec.ts |
24.3s |
| Teller cash management › a cashier is listed, receives an allocation, and settles cash back | teller-cash-management.spec.ts |
22.3s |
| Share product accounting › a share product is mapped to equity and round-trips on edit | share-product-accounting.spec.ts |
21.1s |
| Group membership and lifecycle › a group is activated, staffed, given members and a committee, then emptied | group-membership.spec.ts |
18.2s |
| Group membership and lifecycle › an empty group is closed with a reason, and a group with members is refused | group-membership.spec.ts |
16.9s |
📼 Download the HTML report, videos and traces — see the playwright-report-backend artifact.
Generated by run 34032778412 from 59b72f3. The run executed a fork branch, so treat its contents as unverified.
Aman-Mittal
reviewed
Sep 6, 2026
Member
There was a problem hiding this comment.
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.
Member
Merging This we can handle the changes in separate PR |
Aman-Mittal
approved these changes
Sep 6, 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.
The route puts no constraint on
:variationId, soedit/abcgave+'abc'and leftvariationIdasNaN.isEditModewas set from the presence of the segment, butloadPauseand the save branch both test the id itself, andNaNis falsy. The screen therefore said Edit Interest Pause, the pickers sat on today, and Save posted a create. Nothing on screen said so, which is the part that makes it worth fixing rather than leaving to the API.toRouteIdnow parses the segment and returns null for anything that cannot be a pause id: non-numeric, empty, zero, negative, fractional. An id that fails that check sends the user back to/loans/{loanId}/interest-pauses, the same place Cancel and a successful save go. That felt more honest than quietly rendering the create form under an/edit/URL. If you would rather show an error and stay put, say so and I will change it.The same helper reads
loanId, so a non-numeric loan id no longer producesNaNthere either.Five parameterized cases in the existing spec cover
abc,0,-1,1.5and a blank segment, asserting edit mode stays off, no pause fetch is issued and the navigation happens. Against the current component all five fail:and they pass with the change.
The full
ng run fineract-backoffice-ui:unit-testrun has pre-existing failures inweb-storage.adapter,auth.service,config.service,institution-config.service,navigation-config.service,client-formandhas-institution-feature.directive. They fail the same way onmainwithout this branch, and the count moves a little between runs, so they look environment-dependent rather than related.interest-pause-formis not among them either way. Prettier is clean on both changed files.Closes #512