adamsaghy commented on PR #6477:
URL: https://github.com/apache/fineract/pull/6477#issuecomment-5777818898
@mariiaKraievska Please review my concerns / findings:
1. updateFrom resolves the bucket before the change check
WorkingCapitalLoanAssemblerImpl.java:420-429
if (fromApiJsonHelper.parameterExists(delinquencyBucketIdParamName,
element)) {
final Long bucketId = ...;
final DelinquencyBucket bucket =
resolver.findWorkingCapitalBucketByIdIfProvided(bucketId); // always runs
final Long existingBucketId = ...;
if (!Objects.equals(bucketId, existingBucketId)) { ... }
}
The lookup + type validation runs unconditionally, outside the
!Objects.equals(...) guard. The three sibling blocks around it (breachId,
nearBreachId, delinquencyGraceDays) all gate the lookup on the change check.
Consequence: a WC loan created before this PR with a REGULAR bucket (the old
code was findById(bucketId).orElse(null) with no type check, so this was
reachable) can no longer be modified at all if the client resends the unchanged
delinquencyBucketId — it 400s on a no-op. Same shape for a since-deleted id,
which used to be silently ignored and now throws
DelinquencyBucketNotFoundException.
Moving the resolve inside the if fixes both and matches the surrounding
style. The WC product update path already does this correctly via
isChangeInLongParameterNamed
(WorkingCapitalLoanProductWritePlatformServiceImpl.java:276), as does the LP
update path.
2. Error codes and unused message args
Both new errors pass the bucket id as a defaultUserMessageArgs value, but
neither message has a {0} placeholder, so it's never rendered. The codes also
drop the entity segment that the rest of the codebase uses
(validation.msg.delinquencyBucketId.must.be.regular.type vs. the conventional
validation.msg.loanproduct.delinquencyBucketId.…). The two messages are
otherwise near-identical — one code parameterized by expected type would do.
3. retrieveWorkingCapitalDelinquencyBucketOptions() returns null when empty
WorkingCapitalDelinquencyBucketResolver.java:46-50. This preserves the old
serialization behaviour, but it pushes null-returning into a shared service
method that now has two callers (WC product template and, transitively, the WC
loan template). Returning List.of() and keeping the null-ing at the
response-building boundary would be safer.
4. Duplication
retrieveAllDelinquencyBuckets() and retrieveDelinquencyBucketsByType() are
identical apart from the finder
(DelinquencyReadPlatformServiceImpl.java:105-119). Also
validateIsWorkingCapitalType is public but only called from within its own
class.
5. Test-side nits
DelinquencyBucketResolver.resolveBucketId calls resolve(...) on this, so
Spring's @Cacheable proxy is bypassed — every resolution re-fetches all
buckets. Not a correctness issue.
WorkingCapitalLoanAccount.feature still hardcodes delinquencyBucketId | 1 in
three existing scenarios. Harmless today (buildCreateLoanRequest ignores column
7, and the negative scenario fails in the validator before the assembler), but
the seeded bucket ids are genuinely nondeterministic —
DelinquencyGlobalInitializerStep.initialize() creates BASIC_DELINQUENCY_BUCKET
and WC_DELINQUENCY_BUCKET via ParallelExecutionHelper.runInParallel. Now that
resolveBucketId accepts names, those literals are worth converting before
someone wires column 7 up.
LoanProduct.feature C106741 creates a loan product and leaves it behind; the
WC counterpart C106744 deletes its product.
--
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]
