FINERACT-2471: Implement 'Force Debit' functionality for Savings Accounts with Configurable Limits#5465
Conversation
|
@Saifulhuq01 Please squash and commit. There are recent changes on the Github Actions More info: FINERACT-2177, PR #5431. |
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
from
29516fb to
aff793e
Compare
@IOhacker Thanks for the heads-up regarding FINERACT-2177. I have squashed the changes into a single clean commit and rebased on top of the latest develop to ensure all recent CI fixes are included. The branch is now up-to-date and ready for the workflows to be approved. |
|
Thanks for the clear breakdown, @Saifulhuq01! The Phase 1 implementation looks solid. I have started reviewing the changes—specifically the SavingsAccountForceWithdrawalTest.java and the new permission logic—to ensure my implementation of Phase 2 (Reason Codes & Notification triggers) aligns perfectly with this structure. I'll be ready to open the Phase 2 PR once this is merged. |
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
2 times, most recently
from
077261c to
a4711a7
Compare
@ayushscodes-cs Thanks for the review. Please focus your checks on Ensure that your Phase 2 implementation (Reason Codes/UI) consumes these configurations strictly as defined here, without altering the underlying transaction processing flow. |
|
@Saifulhuq01 please make sure that the title of the PR follows the naming convention it must bd (without double quotes) "FINERACT-2471: Implement 'Force Debit' functionality for Savings Accounts with Configurable Limits" |
|
Hi @IOhacker, Thank you for your guidance! I've squashed all commits into a single GPG-signed commit as requested. The commit is now verified I noticed the "Fineract PR Compliance" check is failing due to the PR title format. The error is: Unfortunately, I cannot edit the PR title through the GitHub UI (the edit button is not available for external contributors on this repository). Could you please update the PR title to: JIRA Ticket: https://issues.apache.org/jira/browse/FINERACT-2471 Thank you! |
IOhacker
changed the title
Aman-Mittal
left a comment
Aman-Mittal
left a comment
There was a problem hiding this comment.
Seems ok overall, kindly address review comments
| } else if (is(commandParam, "withdrawal")) { | ||
| final CommandWrapper commandRequest = builder.savingsAccountWithdrawal(savingsId).build(); | ||
| result = this.commandsSourceWritePlatformService.logCommandSource(commandRequest); | ||
| } else if (is(commandParam, "force-withdrawal")) { |
There was a problem hiding this comment.
This else if is becoming too long i think we can optimise this further
There was a problem hiding this comment.
This else if is becoming too long i think we can optimise this further
@Aman-Mittal Valid point. This class definitely relies heavily on the else-if chain pattern.
However, for this specific PR, I intentionally stuck to the existing pattern to ensure consistency with the surrounding code and to strictly limit the sco0pe of changes to the "Force Withdrawal" feature.
I am hesitant to refactor the command dispatch logic here as it would require touching legacy paths (standard withdrawal/deposit) which increases regression risk. I think a structural refactor would be better suited for a dedicated "Technical Debt" PR.
Does that sound reasonable?
| // TODO: Rewrite to use fineract-client instead! | ||
| // Example: org.apache.fineract.integrationtests.common.loans.LoanTransactionHelper.disburseLoan(java.lang.Long, | ||
| // Example: | ||
| // org.apache.fineract.integrationtests.common.loans.LoanTransactionHelper.disburseLoan(java.lang.Long, |
There was a problem hiding this comment.
This is bringing too much noise in review this comment part can be reverted
There was a problem hiding this comment.
This is bringing too much noise in review this comment part can be reverted
@Aman-Mittal Thanks for the feedback.
I realized that the logging/refactoring changes were adding unnecessary noise to this PR.
I have reverted those commits to keep this PR strictly focused on the 'Force Withdrawal' logic.
The code is now back to the clean state. Ready for re-review.
|
Ok, I think we can make ticket for this type of refractor.
…On Mon, 9 Feb, 2026, 10:24 am Saifulhuq, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/api/SavingsAccountTransactionsApiResource.java
<#5465 (comment)>:
> @@ -193,6 +193,9 @@ public String ***@***.***("savingsId") final Long savingsId, @QueryPa
} else if (is(commandParam, "withdrawal")) {
final CommandWrapper commandRequest = builder.savingsAccountWithdrawal(savingsId).build();
result = this.commandsSourceWritePlatformService.logCommandSource(commandRequest);
+ } else if (is(commandParam, "force-withdrawal")) {
This else if is becoming too long i think we can optimise this further
@Aman-Mittal <https://github.com/Aman-Mittal> Valid point. This class
definitely relies heavily on the else-if chain pattern.
However, for this specific PR, I intentionally stuck to the existing
pattern to ensure consistency with the surrounding code and to strictly
limit the sco0pe of changes to the "Force Withdrawal" feature.
I am hesitant to refactor the command dispatch logic here as it would
require touching legacy paths (standard withdrawal/deposit) which increases
regression risk. I think a structural refactor would be better suited for a
dedicated "Technical Debt" PR.
Does that sound reasonable?
—
Reply to this email directly, view it on GitHub
<#5465 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHV6TA43A7T7M4ANBDGZ6334LAHJLAVCNFSM6AAAAACULZBWYSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTONZRGE2DQMJXGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@Aman-Mittal Agreed. I have created FINERACT-2471 to track this refactoring as a technical debt item. Thanks for the review. |
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
3 times, most recently
from
1a480e4 to
a7097e5
Compare
| <column name="description" value="The maximum negative balance allowed when force withdrawal is enabled."/> | ||
| </insert> | ||
|
|
||
| <insert tableName="m_permission"> |
There was a problem hiding this comment.
FORCE_WITHDRAWAL_SAVINGSACCOUNT_CHECKER permission needs to be implemented as well please
There was a problem hiding this comment.
FORCE_WITHDRAWAL_SAVINGSACCOUNT_CHECKER permission needs to be implemented as well please
@anmotayo Valid point.
I have updated the changelog to include FORCE_WITHDRAWAL_SAVINGSACCOUNT_CHECKER to ensure full compliance with the standard Maker-Checker workflow.
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
4 times, most recently
from
e3ad1fb to
9a85553
Compare
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
Why it contains ".apt_generated" folders ?
Good catch. I noticed those folders hung around due to a git tracking/cache issuue on my end. I'm currently resolving some Cucumber test failures on my local fork to ensure the logic is solid. Once those are green, I'll push the update and the IDE artifacts will be purged. |
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
3 times, most recently
from
95dd749 to
7f5c5bc
Compare
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
2 times, most recently
from
6145b36 to
d201b77
Compare
|
@adamsaghy @anmotayo I have resolved the conflicts, removed the IDE artifacts, and verified the Maker-Checker permissions. The build is fully green in local. I am hoping to get this merged before the Community Call on Monday so I can reference it during the Roadmap discussion regarding the GSoC Idempotency proposal. Thanks!" |
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
6 times, most recently
from
7d0eefb to
271211a
Compare
|
Run './gradlew :fineract-provider:spotlessApply' to fix these violations. |
@IOhacker thanks i got success in my local :
project version: 1.15.0-SNAPSHOT
[Incubating] Problems report is available at: file:///home/sai/Desktop/dev/OpenSource/fineract/build/reports/problems/problems-report.html Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0. You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins. For more on this, please refer to https://docs.gradle.org/8.14.3/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation. BUILD SUCCESSFUL in 29s i find issue inline fn ig ill check and fix that soon :) |
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
2 times, most recently
from
1642898 to
e397ff4
Compare
|
@Saifulhuq01 Please rebase. |
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
from
e397ff4 to
8e0fa85
Compare
|
something went wrong... many conflicts and many file changes :( |
checking @adamsaghy :) |
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
from
8e0fa85 to
9ab3f1c
Compare
|
All requested changes have been implemented and the merge conflicts are resolved :) |
|
@Saifulhuq01 I will try to review it today, but it might only happen tomorrow... |
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
from
6f351e0 to
552f6b3
Compare
|
@Saifulhuq01 There was some issues with CI due to some tests cases. Could you please rebase this PR with latest? The failed checks should go away. |
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
from
552f6b3 to
37bf881
Compare
… Added configs, API support, and fixed Interop compilation errors.
Saifulhuq01
force-pushed
the
FINERACT-2471-force-withdrawal-savings
branch
from
37bf881 to
d90521d
Compare
|
@adamsaghy I dug into the CI pipeline regression on the test-core matrices. The root cause was a BeanCreationException during the Spring ApplicationContext initialization. The new SavingsAccountForceWithdrawalBusinessEvent was not registered in the database, which aborted the Liquibase startup sequence and caused the cascading integration test failures (e.g., relation 'm_loan_transaction' does not exist). Fixes applied in the latest force-push: Event Registration: Added SavingsAccountForceWithdrawalBusinessEvent to the m_external_event_configuration table via the Liquibase changelog to satisfy the validation service. Test Context: Appended the new event to the mocked configuration in ExternalEventConfigurationValidationServiceTest.java. Compliance: Executed Spotless to fix trailing spaces and import orders in SavingsAccountDomainServiceJpa.java. The commit is cleanly squashed and rebased against the latest develop. Monitoring the CI now—once the pipeline goes green, this should be unblocked for your final review and merge. |
6 participants
Description
This PR implements Phase 1 of the "Force Debit" functionality for Savings Accounts, allowing financial institutions to enforce withdrawals even if the account has insufficient funds, up to a globally configurable limit.
This implementation aligns with the community consensus (mailing list discussion Feb 5-6) to use a dedicated command force-withdrawal rather than overloading the standard withdrawal API.
Changes Implemented
Validation
Next Steps (Phase 2)
As discussed, @ayush will pick up Phase 2 to handle:
JIRA
Resolves FINERACT-2471.