MarianaDmytrivBinariks commented on PR #6477:
URL: https://github.com/apache/fineract/pull/6477#issuecomment-5793276419
> > @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.
>
> From 1-4 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.
>
> @MarianaDmytrivBinariks told me she is working on 5th. Please correct me
if I`m wrong
@mariiaKraievska yes, you're right
5.
- DelinquencyBucketResolver.resolveBucketId calls resolve(...) on this, so
Spring's @Cacheable proxy is bypassed — every resolution re-fetches all
buckets. Not a correctness issue. - fixed
- WorkingCapitalLoanAccount.feature still hardcodes delinquencyBucketId | 1
in three existing scenarios. - fixed
- LoanProduct.feature C106741 creates a loan product and leaves it behind;
the WC counterpart C106744 deletes its product. - regular loan product have no
delete endpoint at all, so it can't be done
--
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]
