[I] Rejecting a maker-checker task deletes the record instead of recording the rejection [fineract-backoffice-ui]

["Aman-Mittal (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@fineract.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Rejecting a maker-checker task deletes the record instead of recording the rejection [fineract-backoffice-ui]

["LE0-Lin (via GitHub)" <gi...@apache.org>]

LE0-Lin commented on issue #280:
URL: https://github.com/apache/fineract-backoffice-ui/issues/280#issuecomment-5356857635

   Hi, I noticed the previous main-target PR #325 was closed due to inactivity and this issue remains open. I'd like to take this from the current main branch. I'll keep the change scoped to the reject command, the standard confirmation/i18n flow, and Vitest regression coverage described here, then run the full relevant checks. Thanks!


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Rejecting a maker-checker task deletes the record instead of recording the rejection [fineract-backoffice-ui]

["Aman-Mittal (via GitHub)" <gi...@apache.org>]

Aman-Mittal closed issue #280: Rejecting a maker-checker task deletes the record instead of recording the rejection
URL: https://github.com/apache/fineract-backoffice-ui/issues/280


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org