Aman-Mittal opened a new issue, #280:
URL: https://github.com/apache/fineract-backoffice-ui/issues/280
## Business value
The checker inbox approves and rejects through two different verbs, and only
one of them is right.
`src/app/features/tasks/checker-inbox/checker-inbox.component.ts:143`
(approve) does the correct thing:
```ts
this.makerCheckerService.postMakercheckersAuditId(task['id'] as number,
'approve')
```
`:156` (reject) does not:
```ts
this.makerCheckerService.deleteMakercheckersAuditId(task['id'] as number)
```
The generated client documents the two endpoints as distinct operations:
```
post /v1/makercheckers/{auditId} Approve Maker Checker Entry | Reject
Maker Checker Entry
delete /v1/makercheckers/{auditId} Delete Maker Checker Entry
```
Verified against a live backend — `command=reject` is a recognised command,
and the platform enumerates its accepted values when you send a bad one:
```
POST /v1/makercheckers/1?command=totallyNotACommand 400 "unsupported
value of: totallyNotACommand"
POST /v1/makercheckers/1?command=reject 404 "Audit with
identifier 1 is Not Awaiting Approval"
```
The second response is the proof: `reject` passed command validation and
reached the audit lookup.
**Why this matters more than a wrong verb usually would.** Maker-checker
exists to produce an audit trail: who proposed a change, who reviewed it, and
what they decided. Approving records an approval. Rejecting is supposed to
record a rejection against the same entry. Deleting removes the entry, so the
record of the proposal *and* the decision disappears together. An auditor
asking "who rejected this loan write-off, and when?" has nothing to look at,
and the UI reports "Task rejected successfully" either way — so nobody
discovers the loss until it is needed.
For an institution using four-eyes approval to satisfy a regulator, a
rejection path that destroys evidence is a control failure, not a cosmetic bug.
## Reproducing it
```
grep -n "postMakercheckersAuditId\|deleteMakercheckersAuditId" \
src/app/features/tasks/checker-inbox/checker-inbox.component.ts
```
Two call sites, two different endpoints, one operation.
## Describing the change
```ts
// before
this.makerCheckerService.deleteMakercheckersAuditId(task['id'] as
number).subscribe({ ... });
// after
this.makerCheckerService.postMakercheckersAuditId(task['id'] as number,
'reject').subscribe({ ... });
```
That is the fix. Two further things in the same handler are worth doing
while you are there, and both are small:
1. **`confirm()`.** `:155` uses the browser's native confirm box. It cannot
be translated, cannot be styled, and cannot be reached from the e2e suite.
Replace it with `DialogService.confirm({ title, message, destructive: true })`
— the pattern is in
`src/app/features/groups/tabs/group-notes-list.component.ts:166`. The same
substitution is tracked elsewhere for other screens (#229–#232); this call site
is not in any of them.
2. **Hardcoded English.** `'Task rejected successfully'` and `'Failed to
reject task'` bypass translation. Add keys alongside the existing ones.
Do all three in one PR; they are the same twelve lines.
## Testing
Unlike most issues here there is no cucumber scenario to lift — the
platform's acceptance suite exercises maker-checker through the API rather than
as a user-facing flow. So the test has to assert the contract directly:
**A unit spec** in a new `checker-inbox.component.spec.ts`. There is
currently none, and the assertion is exactly the thing that was wrong:
```ts
it('rejects through the reject command, not by deleting the entry', async ()
=> {
adapters.overlay.nextModalResult = true; // confirm
await component.onReject({ id: 7 });
expect(serviceSpy.postMakercheckersAuditId).toHaveBeenCalledWith(7,
'reject');
expect(serviceSpy.deleteMakercheckersAuditId).not.toHaveBeenCalled();
});
```
The negative assertion matters. A spec that only checks the POST would still
pass if both calls fired.
Also cover: cancelling the confirmation issues no call at all.
Use `provideFakeAdapters()` from `src/app/testing/adapters.ts` and
`provideTranslateTesting()` from `src/app/testing/i18n-testing.ts` — importing
`@ngx-translate/core` directly in a spec is banned by lint (ADR-0003) and CI
will fail on it.
**A mocked e2e** is welcome: intercept `POST /makercheckers/*`, assert the
`command` search parameter is `reject`, and fail the test if any `DELETE`
reaches that path. See the `Probe` pattern in
`e2e/client-servicing-gaps.spec.ts`.
## Scope
In scope: the reject verb, the confirm dialog, and the two hardcoded strings
in this handler.
Out of scope: everything else about the checker inbox — it also has no
search, no filtering and no bulk approve/reject, which is a much larger piece
of work and deserves its own issue.
## Getting started
- `src/app/features/tasks/checker-inbox/checker-inbox.component.ts`
- Confirmation pattern:
`src/app/features/groups/tabs/group-notes-list.component.ts:165-183`
- Spec harness: `src/app/testing/adapters.ts`,
`src/app/testing/i18n-testing.ts`
- `npm test` and `npm run lint:prune` must pass. Run `lint:prune` *after*
writing the spec, not before — it prunes suppressions and will fail on a spec
that imports the translate module directly.
--
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]
