Skip to content

FINERACT-2455: Add WC business events for transactions - #6300

Draft
oleksii-novikov-onix wants to merge 1 commit into
apache:developfrom
openMF:FINERACT-2455/wc-transaction-business-events
Draft

FINERACT-2455: Add WC business events for transactions#6300
oleksii-novikov-onix wants to merge 1 commit into
apache:developfrom
openMF:FINERACT-2455/wc-transaction-business-events

Conversation

@oleksii-novikov-onix

Copy link
Copy Markdown
Contributor

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!

  • Write the commit message as per our guidelines
  • Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
  • Create/update unit or integration tests for verifying the changes made.
  • Follow our coding conventions.
  • Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
  • This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
  • If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

@oleksii-novikov-onix
oleksii-novikov-onix force-pushed the FINERACT-2455/wc-transaction-business-events branch from 2128ae1 to 5a8edd4 Compare August 20, 2026 14:11

@galovics galovics left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants