FINERACT-2455: Add WC business events for transactions - #6300
FINERACT-2455: Add WC business events for transactions#6300oleksii-novikov-onix wants to merge 1 commit into
Conversation
2128ae1 to
5a8edd4
Compare
galovics
left a comment
There was a problem hiding this comment.
Nice piece of work overall, the event model is clearly better than the TransactionReversed catch-all it replaces. A few things need fixing before this can merge though.
1. Liquibase deletes existing m_external_event_configuration rows instead of renaming (parts/0071_wc_transaction_business_events.xml, changesets wcl-0071-4 and wcl-0071-11)
<changeSet author="fineract" id="wcl-0071-4">
<delete tableName="m_external_event_configuration">
<where>type = 'WorkingCapitalLoanTransactionReversedBusinessEvent'</where>
</delete>
</changeSet>Rows must never be deleted from this table - every other rename in this file (WrittenOff -> WriteOffTransaction, UndoWrittenOff -> UndoWriteOffTransaction, ChargeAdjustmentPost -> ChargeAdjustmentTransaction) is done as an UPDATE so the enabled flag survives. This one does delete + fresh insert with enabled=false, so any tenant that had WorkingCapitalLoanTransactionReversedBusinessEvent enabled silently loses it after upgrade. Please make it an UPDATE rename like the others. wcl-0071-11 (ChargeAdjustmentPre) has no successor at all - it should stay in place (disabled), not be deleted.
2. Breaking change inside an already-published Avro V1 schema (WorkingCapitalLoanTransactionDataV1.avsc)
-"org.apache.fineract.avro.loan.v1.LoanTransactionEnumDataV1"
+"org.apache.fineract.avro.generic.v1.StringEnumOptionDataV1"
This mutates the payload of every existing WC transaction event (Disbursal, Repayment, ChargeOff, DiscountFee, ...), not just the new ones, and it now diverges from LoanTransactionDataV1, which still uses LoanTransactionEnumDataV1 for the same concept. A V1 schema is a wire contract - this needs either a V2 record or an explicit, documented breaking-change call.
3. Non-reentrant recording window (WorkingCapitalLoanAdjustTransactionEventPublisher#publishReprocessed)
businessEventNotifierService.startExternalEventRecording();
adjustments.forEach(... notifyPostBusinessEvent ...);
businessEventNotifierService.stopExternalEventRecording();eventRecordingEnabled is a plain ThreadLocal<Boolean>, not a counter, so this doesn't nest. COBBusinessStepServiceImpl.run() already opens a recording window around the whole business step chain when bulk events are enabled. If WC transaction reprocessing is ever reached from inside a COB step, the inner stopExternalEventRecording() flushes and closes COB's window mid-chain - premature bulk event, and nothing recorded afterwards. Latent today (only command paths call reprocessing), but worth guarding or making the window reentrant centrally - ReplayedTransactionBusinessEventServiceImpl in fineract-loan has the same shape, so this may be worth fixing once for both.
4. WorkingCapitalLoanChargeAdjustmentPreBusinessEvent removed silently (WorkingCapitalLoanChargeWritePlatformServiceImpl#adjustmentForLoanCharge)
The rest of the WC events follow the Pre/Post pattern; this drops the Pre half (and its config row) with no mention in the PR description. If deliberate, please call it out explicitly.
Smaller things:
isPostingEnabled()probes config by constructing a throwaway event with anullpayload - works by accident, fragile ifgetType()ever changes. An overload taking the event type directly would be cleaner.- No unit tests for the new
WorkingCapitalLoanAdjustTransactionEventPublisher(posting-disabled short circuit, empty adjustments, exception path) or the new serializer's null-payload case. snapshots()reads post-replay charge-paid-by rows via a Spring Data query right afterreprocessFullydeletes/rebuilds them - relies on implicit auto-flush; worth an explicit flush for safety.
Recommendation: CHANGES_REQUESTED
Description
Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.