oleksii-novikov-onix commented on PR #6480:
URL: https://github.com/apache/fineract/pull/6480#issuecomment-5812796619
> Routing, permissions, transaction-type checks and the COB filter all hold
up, and the e2e coverage is good. No new JAX-RS routes are added - the four
`transactions/{transactionId}` and
`transactions/external-id/{transactionExternalId}` POST endpoints already
existed for `undo` and just gain two more `command` values, and the builders
use the same `DISCOUNTFEE`/`DISCOUNTFEEADJUSTMENT` + `WORKINGCAPITALLOAN` pair
as the loan-level ones, so the same permissions apply. The loan and transaction
external ids aren't mixed up (the transaction is loaded with
`findByIdAndWcLoan_Id`), a wrong-type transaction is rejected in both
directions, the duplicate discount fee guard still applies, and the COB filter
is unaffected: `isExternal` now uses the anchored
`EXTERNAL_ID_LOAN_PATH_PATTERN`, so `/{loanId}/transactions/external-id/{x}`
resolves the numeric loan id (the #6351 issue), covered by
`WorkingCapitalLoanCOBApiFilterTest`.
>
> Things I'd change:
>
> * **A missing/foreign transaction id in the path returns 400, while the
external-id form and the GET return 404.** `resolveTransactionId` returns a
supplied `transactionId` without checking it belongs to the loan, so an unknown
one reaches the service and fails with a 400 `...transaction.not.found`; the
docs and e2e tests lock the 400 in. 404 is the usual convention for a path
resource - reuse `findByIdAndWcLoan_Id` in `resolveTransactionId` and throw the
not-found exception.
> * **The handlers pick the flow by whether `loanId` happens to be set**
(`WorkingCapitalLoanDiscountFeeCommandHandler` / `...AdjustmentCommandHandler`:
`Optional.ofNullable(command.getLoanId())`). It works only because the
loan-level builders don't set `loanId`; other WC builders do, so if someone
adds it "for consistency" every body-based call silently switches to the path
form and the loan id is treated as a disbursement transaction id. Use an
explicit signal (a dedicated action, or the transaction id on the wrapper) and
add a unit test per branch.
> * **Pending maker-checker rows record a transaction id as the loan's
resource id** (`entityName = WORKINGCAPITALLOAN`, `entityId = transactionId` in
the new builders); `updateForAudit` corrects it afterwards, but pending rows
are misleading.
> * Docs say a non-positive `relatedResourceId` gets `...not.a.number`, but
`longGreaterThanZero` returns `...not.greater.than.zero` for `0`/`-1`; and the
new "no `relatedResourceId` in the body of a path form" check has no test.
> * The non-numeric `relatedResourceId` validation now also changes the
error on the existing body-based endpoint (probably an improvement, but worth
noting), and adding `externalLoanId` to the transaction data/response/Avro
isn't needed for this ticket (and is inserted mid-record) - consider its own
ticket.
>
> Recommendation: COMMENT
1. Missing or foreign transaction id in the path returns 400 instead of 404
- fixed, resolveTransactionId now looks it up with findByIdAndWcLoan_Id, and
undo gets the same treatment since it shares that method.
2. Handlers pick the flow by whether loanId happens to be set - fixed, the
path builders now put the transaction id in subentityId and keep entityId =
loanId, the handlers branch on that, and both branches have a unit test.
3. Pending maker-checker rows record a transaction id as the loan's resource
id - fixed by the same change, entityId is the loan id now, which is what
entityName = WORKINGCAPITALLOAN promises.
4. Docs claim not.a.number for a non-positive relatedResourceId, and the
path-form body check has no test - the docs were already corrected, and the
check is now covered in the existing validator unit test.
5. The non-numeric relatedResourceId error changed on the existing body
endpoint - intentional, a UUID there used to end in a 500 and is now rejected
as a validation error.
6. externalLoanId is out of scope here - deliberate: the transaction payload
identified the loan only by the numeric wcLoanId, while the legacy loan
transaction and the WC charge payloads already carry externalLoanId, so a
consumer working with UUIDs could not resolve the loan from an event.
--
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]
