oleksii-novikov-onix commented on PR #6650:
URL: https://github.com/apache/fineract/pull/6650#issuecomment-6268044020
> ## Review
> Hi @oleksii-novikov-onix, thanks for the PR. I know it's still a draft,
but `build-core` is red, so everything after it got skipped and none of the
integration/e2e/cucumber suites have run yet. Pls get it green first. The event
flow itself looks reasonable to me.
>
>
`fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/messaging/EventAssertion.java:182`:
>
> ```java
> + } catch (ConditionTimeoutException expected) {}
> ```
>
> > This is what breaks the build, errorprone `EmptyCatch` fails
`:fineract-e2e-tests-core:compileTestJava`. Put a short comment in the catch
block saying the timeout is the expected outcome, like `assertEventNotRaised`
does a few lines above.
>
>
`fineract-working-capital-loan/.../service/WorkingCapitalLoanChargeWritePlatformServiceImpl.java:286`:
>
> ```java
> chargePaidByRepository.saveAll(paidByRows);
> + chargePaidByRepository.flush();
> ```
>
> > Why the flush? `WorkingCapitalLoanTransactionDataFactory` builds the
snapshots from the entities in memory, so as far as I can tell the event
doesn't need it. If something does, pls add a comment saying what.
>
> `WorkingCapitalLoanWritePlatformServiceImpl.java`
(`adjustRepaymentLikeTransaction`, `undoRequiresReplay` / `replayAdjustment`):
>
> > Undo plus the new transaction is now one replay from `min(newDate,
reversedDate)`, and the non-replay branch keeps the old flow, so we have two
paths for the same command. The merged path also runs `determineAndTransition`,
`reprocessDelinquencySchedule`, `recalculateOverpaidOnDate` and
`recalculateSettlementDates` 2-3 times (in `completeUndo`, in
`completeReplayedRepaymentLikeTransaction`, then again after). Harmless, but
this is the riskiest part of the PR and it's only covered by e2e scenarios that
haven't run, so I'd like to see it green before judging it.
>
> Smaller ones:
>
> * No unit test for the new `publishReprocessed(Long, Map, List)` overload
in `WorkingCapitalLoanAdjustTransactionEventPublisherTest`, it has the
reversal-vs-pair mapping and the "drop if no pre-snapshot" filter. Also the
lifted charge-off reversal used to go out unconditionally through
`publishReversal` and now only if it's in `preSnapshots`. Holds today since a
charge-off always has an allocation, but it's an implicit dependency.
> * `AdjustTransactionEventGating.feature`,
`WorkingCapitalTransactionReprocessing.feature` and
`WorkingCapitalLoanTransactionAdjustment.feature` have TestRail-tagged
scenarios rewritten (extra repayment, CBR close instead of full repayment). The
matching TestRail cases need updating.
> * `FeignWorkingCapitalLoanCreditBalanceRefundReprocessingTest` enables
three external events and disables them in `finally`. Restore the previous
state instead.
> * `touchedAllocations` can hold the same allocation twice and the filter
uses `List.contains`, a `Set` states the intent better.
> * The PR description is the empty template, pls fill it in before taking
it out of draft.
>
> Recommendation: REQUEST_CHANGES
Thanks for the review @galovics
1. EmptyCatch, build-core - fixed.
2. Paid-by flush - removed. The paid-by list comes from a JPQL query in the
read-write transaction of createLoanCharge, and EclipseLink flushes before it
in AUTO mode.
3. Two adjust paths - no change. The merged path no longer calls
completeUndo, so the transition and the delinquency rebuild run once. Overpaid
and settlement recalculations run twice, against three times on develop. They
are cheap, and dropping them means branching the shared tail or editing
develop’s adjust block. The split mirrors the existing undo: unwind in place,
otherwise replay. Covered by C111084, C111006, C89812 and C106808.
4. Unit tests for the Map overload - added. They cover pairing, reversals,
skipping and the recording window.
5. Lifted charge-off - fixed. Reversals no longer need a pre-snapshot.
6. TestRail - QA will update the 13 affected cases.
7. IT - restores the previous event states in finally.
8. touchedAllocations - now a LinkedHashSet.
9. PR description - filled in.
--
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]
