FINERACT-2545: Implement client charge inactivation#5655
FINERACT-2545: Implement client charge inactivation#5655AshharAhmadKhan wants to merge 1 commit intoapache:developfrom
Conversation
|
Pushed a follow-up commit after deeper review of state transitions and API completeness. The first commit fixed the core bug - the missing handler and stub service. While verifying the full operation lifecycle I found three additional issues worth addressing in the same PR rather than leaving as separate tickets.
Updated the Tests added for the undo guard and the fields filter. All existing tests untouched. |
POST /clients/{clientId}/charges/{chargeId}?command=inactivate was
broken end-to-end. The API accepted the command and built a
CommandWrapper with entity=CLIENTCHARGE action=INACTIVATE, but no
command handler was registered for that key, causing
CommandHandlerProvider to throw UnsupportedCommandException on every
call. The service method was also a stub returning null with a
'functionality not yet supported' comment.
This commit completes the originally intended implementation:
- Add InactivateClientChargeCommandHandler wired to CLIENTCHARGE|INACTIVATE
- Add ClientCharge.inactivate() domain method setting status=false
and inactivationDate
- Implement ClientChargeWritePlatformServiceImpl.inactivateCharge()
with guards for already-inactive, already-waived, and already-paid
- Add integration test covering success path and idempotency failure
FINERACT-2545: Fix state integrity and documentation gaps
Three follow-up fixes identified through deep domain analysis:
1. Add isActive and inactivationDate to CLIENT_CHARGES_RESPONSE_DATA_PARAMETERS
so these fields are returned when using the ?fields= query parameter filter.
2. Fix ClientCharge.undoPayment() and undoWaiver() to not restore active
status when a charge has been explicitly inactivated (inactivationDate != null).
Previously, undoing a payment on an inactivated charge would silently
reactivate it, violating state integrity.
3. Add service-level guard in ClientTransactionWritePlatformServiceJpaRepositoryImpl
to block undo operations on inactive charges entirely, enforcing inactivation
as a terminal state at both domain and service layers.
4. Update @operation Swagger annotation on payOrWaiveClientCharge to document
the inactivate command alongside pay and waive.
Tests added: undo blocked on inactive charge, isActive returned correctly
via explicit ?fields= filter.
7524369 to
fa37023
Compare
|
All checks should now be passing after squashing into a single commit. Happy to make any adjustments based on feedback or direction from the ongoing discussion. |
|
Hi, I went through the PR and the implementation looks well-structured, especially the addition of the command handler and the service-level validations. I noticed that the implementation correctly handles state transitions (like preventing undo operations on inactive charges), which is great for maintaining consistency. One suggestion: it might be helpful to document the expected lifecycle of a client charge (active → inactive → restricted operations) in the API documentation or developer docs, so new contributors can better understand these constraints. Also, since this feature was previously unimplemented despite the infrastructure being present, adding a brief note in the documentation about this behavior could improve clarity. Overall, the changes look solid. Happy to help further with testing or documentation if needed. |
|
Thanks for taking the time to review this and for the thoughtful suggestion, Dishita. I agree that documenting the client charge lifecycle constraints would make the behavior clearer for future contributors. I’m currently waiting for the ongoing discussion and mentor direction on the PR. If we decide to extend the documentation side, I’d be happy to take your help on that. |
|
Thanks for your response! That makes sense. I’d be happy to help with documenting the client charge lifecycle if the documentation side is extended. I’ve been exploring the lifecycle constraints while reviewing this PR and would be glad to contribute to improving clarity for future contributors. |
|
@AshharAhmadKhan @Dishitasuyal You should raise this topic for discussion on the Apache Fineract DEV mail list! |
Thanks @adamsaghy — already raised this on the DEV list and received initial thoughts there as well. At this point I’m mainly waiting on direction guidance from that thread before taking the PR further. DEV thread: https://lists.apache.org/thread/lhh0hx0t2y7vzdrs98fg3qyfx1xo9d0y |
You might want to remind Bharath and Victor to come to a conclusion and provide you feedback on this! |
Thanks @adamsaghy, I followed up on this with James over Matrix, and his view is that the preferred fee model is account-associated. At this point, I’m just waiting for final guidance on whether this should be removed entirely or simply left as is. Based on that, I’ll either update this PR accordingly or close this one and open a cleanup PR if that’s the preferred direction. |
|
Thanks, Adam. I discussed this with James and have been looking into it more thoroughly since getting his input. Once I’ve finished the research, I’ll follow up on the DEV list with the findings before taking the PR further. |
|
Closing this in favour of #5764 which removes the write path instead of implementing it, per the direction from the DEV list discussion. See https://lists.apache.org/thread/lhh0hx0t2y7vzdrs98fg3qyfx1xo9d0y for context. |
Description
POST /clients/{clientId}/charges/{chargeId}?command=inactivatehas never worked. The API accepted the command and built aCommandWrapperwithentity=CLIENTCHARGEandaction=INACTIVATE, but no command handler was ever registered for the key"CLIENTCHARGE|INACTIVATE". The dispatcher threwUnsupportedCommandExceptionimmediately, returning HTTP 500 to every caller. The service methodClientChargeWritePlatformServiceImpl.inactivateCharge()was also a stub returningnullwith the comment// functionality not yet supported.All the surrounding infrastructure was already in place — the DB columns
is_activeandinactivated_on_dateexist inm_client_charge, the permissionINACTIVATE_CLIENTCHARGEis seeded in0002_initial_data.xml, the builder methodinactivateClientCharge()exists inCommandWrapperBuilder, and the API routing atClientChargesApiResourceline 212 correctly accepts the command. The handler and service implementation were simply never completed.Savings charges have had a fully working equivalent (
InactivateSavingsAccountChargeCommandHandler) since MIFOSX-1408. Client charges were left behind.Changes:
InactivateClientChargeCommandHandler.java— new handler registered underCLIENTCHARGE|INACTIVATE, modelled onWaiveClientChargeCommandHandler.ClientCharge.java— addedinactivate(LocalDate)domain method settingstatus = falseandinactivationDate. The DB columns already existed, the method was simply missing.ClientChargeWritePlatformServiceImpl.java— replaced thereturn nullstub with a full implementation. Guards for already-inactive, already-waived, and already-paid states, followed byclientCharge.inactivate()andsaveAndFlush.ClientHelper.java— addedinactivateChargesForClients()andgetClientChargeField()test helpers.ClientChargesTest.java— addedclientChargeInactivateTest()covering the success path, theisActivefield assertion post-inactivation, and the idempotency failure (inactivating an already-inactive charge must return 400).Checklist