mariiaKraievska commented on PR #6477:
URL: https://github.com/apache/fineract/pull/6477#issuecomment-5793078780
> @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.
All addressed, thanks:
1. moved bucket validation inside `isChangeInLongParameterNamed`, same
pattern as breach/nearBreach and WC product update
2. Error codes — shared message template parameterized by expected type
3. null from resolver— `retrieveWorkingCapitalDelinquencyBucketOptions()`
returns the list as-is. empty → `null` only at the product template response.
4. Duplication — `retrieveAll` / `byType` share `mapAndEnrich`.
`validateIsWorkingCapitalType` is private.
--
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]
