adamsaghy commented on PR #6300: URL: https://github.com/apache/fineract/pull/6300#issuecomment-5454953559
> > 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`)** > > ```java > > 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 a `null` payload - works by accident, fragile if `getType()` 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 after `reprocessFully` deletes/rebuilds them - relies on implicit auto-flush; worth an explicit flush for safety. > > > > Recommendation: CHANGES_REQUESTED > > 1. Fixed for the renames: wcl-0070-3-rename now does the UPDATE first, then a guarded INSERT, so enabled survives. Same shape as the other renames in the file. > > ChargeAdjustmentPre I'd like to keep as a delete, though. That event was added without a reason and never externalized in the first place - Pre events don't reach postEvent at all - and the class is now gone. > > 2. StringEnumOptionDataV1 is gone. The field now uses a WC-owned WorkingCapitalLoanTransactionTypeDataV1 with the same shape as LoanTransactionEnumDataV1, so the divergence from LoanTransactionDataV1 is resolved. It's still an in-place edit rather than a V2 record, I dont think we need introduce V2. > 3. Agreed, but the shape is mirrored from ReplayedTransactionBusinessEventServiceImpl - diverging here would make WC and plain loans behave differently for the same replay. I think we need a separate investigation, separate PR. > 4. It wasn't half of a pattern - it was the only notifyPreBusinessEvent call and the only *PreBusinessEvent class in the whole WC module, with no listener registered for it anywhere. Pre events also never reach postEvent, so the config row was dead from the day it was added. Removed rather than kept for symmetry with a pattern WC doesn't use. @oleksii-novikov-onix I believe Arnold is right, lets stick with `LoanTransactionEnumDataV1`, just extend it with the missing transaction types. @galovics I believe Oleksii addressed all your concerns. -- 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]
