Aman-Mittal commented on code in PR #204:
URL:
https://github.com/apache/fineract-consumer-facing/pull/204#discussion_r4293020453
##########
consumer/src/main/java/org/apache/fineract/consumer/transfers/command/service/TransfersCommandServiceImpl.java:
##########
@@ -213,6 +219,20 @@ private void requireDestinationAllowed(Jwt jwt, Long
userId, Long toAccountId, b
}
}
+ private void requireWithinOutstanding(Jwt jwt, Long loanId, BigDecimal
amount) {
+ LoanAccountQueryData loan = loansQueryService.getLoan(jwt, loanId);
Review Comment:
**Non-functional: this call is much heavier than the check needs.**
`LoansQueryServiceImpl.getLoan`:
- calls `retrieveOneLoan(loanId, false, "repaymentSchedule,transactions",
...)`, so the full schedule and transaction history are fetched and mapped just
to read `active` and `totalOutstanding`. On a long-running loan that's a large
payload, and it happens twice per transfer (once in `initiate`, once in
`confirm`);
- runs `authorize(LOANS_VIEW, ...)` again, right after the `ownsResource`
check on L203;
- throws `loans.query` exceptions (`LoanQueryAccessDeniedException`, the
loan-not-found and upstream exceptions). These come back from `/transfers/*`
with `error.msg.consumer.loans...` codes instead of transfer codes;
- adds a `transfers.command → loans.query` module dependency. Modulith
allows it, but this is the only reason for it.
A narrow lookup that returns only status and the payoff amount, without
associations, would avoid all four problems. It also fits the refactor I
suggested in reply to your architecture question, so the new code goes in the
infrastructure lookup rather than a dependency on `LoansQueryService`.
##########
consumer/src/main/java/org/apache/fineract/consumer/transfers/command/service/TransfersCommandServiceImpl.java:
##########
@@ -213,6 +219,20 @@ private void requireDestinationAllowed(Jwt jwt, Long
userId, Long toAccountId, b
}
}
+ private void requireWithinOutstanding(Jwt jwt, Long loanId, BigDecimal
amount) {
+ LoanAccountQueryData loan = loansQueryService.getLoan(jwt, loanId);
+ if (loan == null || !loan.isActive()) {
+ throw new TransferLoanNotActiveException();
+ }
+ BigDecimal outstanding = loan.getTotalOutstanding();
+ if (outstanding == null || outstanding.signum() <= 0) {
+ throw new TransferLoanNotActiveException();
+ }
+ if (amount.compareTo(outstanding) > 0) {
Review Comment:
**Correctness: `totalOutstanding` is the wrong cap for some products.** You
flagged this in the PR description, and I think it needs fixing before merge
rather than leaving as a known gap.
For loans with interest recalculation (declining balance, progressive
schedules), `summary.totalOutstanding` includes interest for **all future
periods**. If the customer pays exactly `totalOutstanding` today, Fineract
recalculates interest as of the repayment date, the unearned interest goes
away, and the loan ends up **overpaid**. That is the exact outcome #114 is
trying to prevent, and those are the products customers most often pay off
early.
Fineract has an endpoint for this: `GET
/loans/{loanId}/transactions/template?command=prepayLoan` returns the amount
needed to close the loan **as of today**. Capping at that value works for flat
and recalculating products alike. For products without recalculation the two
amounts are the same, so nothing changes for the cases that already work.
At minimum, please add a unit test for each product type, plus a comment
explaining why the chosen field is correct.
##########
docs/consumer/features/transfers.adoc:
##########
@@ -144,5 +144,5 @@ The query side owns transfer history because it is a
cross-account view: per-acc
* *Transfers execute immediately.* They use the server's current date; there
is no scheduling, no future-dated transfer, and no recurring transfer.
* *The transfer description is a fixed constant.* The customer cannot attach a
note.
-* *No amount ceiling on own-account transfers.* Transfers to the customer's
own accounts are bounded only by the optional per-beneficiary `transferLimit`.
A global or per-tier transaction limit would need new policy data.
+* *No amount ceiling on own-savings transfers.* Transfers between the
customer's own savings accounts are bounded only by the optional
per-beneficiary `transferLimit` (loan repayments are capped at the loan's
outstanding balance). A global or per-tier transaction limit would need new
policy data.
Review Comment:
**E2E coverage.** The checklist says e2e coverage was added where relevant,
but `transfers.feature` has no scenario for the new rule. Two cheap scenarios
against the real Fineract stack would cover it:
- `Initiating a loan repayment above the outstanding balance is rejected`:
the seeded loan principal is 10,000, so initiate 10,001 and expect 422 with
`error.msg.consumer.transfers.command.loan.overpayment`. No OTP is sent and no
money moves.
- `Repaying exactly the outstanding balance is accepted`: this proves the
boundary against real Fineract numbers. The savings account would need funding
up to the outstanding amount, or use a smaller seeded loan.
This also tests the `totalOutstanding` mapping against real Fineract data,
which the mocked unit tests can't do.
Also, the `confirm`-time check isn't atomic with the transfer. Could you add
a sentence here saying the cap is a best-effort UX guard and not a guarantee?
(See my comment on `confirm`.)
##########
consumer/src/main/java/org/apache/fineract/consumer/transfers/command/service/TransfersCommandServiceImpl.java:
##########
@@ -70,6 +74,7 @@ public class TransfersCommandServiceImpl implements
TransfersCommandService {
private final UserQueryService userQueryService;
private final AccessPolicyEvaluator accessPolicyEvaluator;
private final BeneficiariesQueryService beneficiariesQueryService;
+ private final LoansQueryService loansQueryService;
Review Comment:
This new constructor dependency is what trips
`featuresDoNotCallFineractFeignClientsDirectly`. The store records the whole
constructor signature, so the existing violation lines for this class no longer
match. The architecture question is answered in a separate comment. In short:
please don't edit the store or switch to setter injection; do the preparatory
refactor.
##########
frontend/public/i18n/en.json:
##########
@@ -570,5 +570,7 @@
"error.msg.consumer.openbanking.consent.invalid": "This consent does not
permit that request.",
"error.msg.consumer.openbanking.account.not.found": "Account not found.",
"error.msg.consumer.authentication.kyc.revoked": "Your identity verification
is no longer valid. Please contact support.",
- "error.msg.consumer.summary.upstream.unavailable": "Account summary is
temporarily unavailable."
+ "error.msg.consumer.summary.upstream.unavailable": "Account summary is
temporarily unavailable.",
+ "error.msg.consumer.transfers.command.loan.overpayment": "The amount exceeds
your loan's outstanding balance.",
+ "error.msg.consumer.transfers.command.loan.not.active": "This loan has no
outstanding balance to repay."
Review Comment:
`TransferLoanNotActiveException` is thrown both for `!loan.isActive()`
(submitted, approved but not disbursed, closed, written off) and for a zero
balance. "This loan has no outstanding balance to repay." is wrong for a loan
that is pending approval or not yet disbursed.
Either reword it to cover both cases (e.g. "This loan can't receive a
repayment right now."), or split it into two codes (`loan.not.active` and
`loan.fully.repaid`). The same applies to the Hindi string and to the
exception's message: "Loan is not active or has no outstanding balance." is
fine as a developer message.
##########
consumer/src/main/java/org/apache/fineract/consumer/transfers/command/service/TransfersCommandServiceImpl.java:
##########
@@ -198,6 +203,7 @@ private void requireDestinationAllowed(Jwt jwt, Long
userId, Long toAccountId, b
if (!accessPolicyEvaluator.ownsResource(jwt, ResourceType.LOANS,
toAccountId)) {
throw new TransferAccessDeniedException();
}
+ requireWithinOutstanding(jwt, toAccountId, amount);
Review Comment:
**Idempotency / retry behavior.** `confirm` calls this check (L138) before
it sets the idempotency key (L141), so the check runs before Fineract can
replay the original result. Consider the case where the customer pays the full
balance and the response is lost on a timeout:
- The client retries with the same `Idempotency-Key`. Because the OTP was
consumed (`OtpServiceImpl` deletes it on success), the retry must go through
`initiate` again.
- `initiate` now throws `TransferLoanNotActiveException` (the loan is
closed). For a partial payment where `amount > remaining`, it throws
`TransferLoanOverpaymentException` instead.
So the customer is told their payment was rejected when it actually
succeeded. Before this PR the retry reached Fineract, which replayed the
original response.
This isn't a blocker, since the check at `initiate` is the main point of the
PR. But please:
1. document this in `transfers.adoc` (the check is best-effort and not
atomic with the transfer: two confirms with different keys can each pass
against the same balance, and Fineract would accept both), and
2. consider whether the `confirm`-time check should only reject inactive
loans and leave the amount check to `initiate`. That keeps the
time-of-check/time-of-use gap the same, since the check was never atomic, and
makes retries less surprising.
--
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]
