Josue Araujo created FINERACT-2934:
--------------------------------------
Summary: Merge the two force-withdrawal global configurations into
a single one
Key: FINERACT-2934
URL: https://issues.apache.org/jira/browse/FINERACT-2934
Project: Apache Fineract
Issue Type: Improvement
Components: Savings, System
Affects Versions: 1.15.0, 1.16.0
Reporter: Josue Araujo
Fix For: 1.16.0
h2. Background
FINERACT-2471 introduced force withdrawals on savings accounts, controlled by
two global configurations (migration {{0217_force_withdrawal_configs.xml}},
released in 1.15.0):
* {{allow-force-withdrawal-on-savings-account}}
* {{force-withdrawal-on-savings-account-limit}}
h2. Observation
The two configurations are redundant. In the code, only one attribute of each
is ever read:
* {{ConfigurationDomainServiceJpa#isForceWithdrawalOnSavingsAccountEnabled()}}
reads {{isEnabled()}} of {{allow-force-withdrawal-on-savings-account}}.
*
{{ConfigurationDomainServiceJpa#retrieveForceWithdrawalOnSavingsAccountLimit()}}
reads {{getValue()}} of {{force-withdrawal-on-savings-account-limit}}.
The value of the first one and the enabled flag of the second one are never
used. Both are inserted with value=0 and enabled=false, so an administrator has
to set two separate rows (and the "enabled" flag of the limit row, although it
has no effect, is easy to misread as meaningful).
h2. Proposal
Keep a single configuration, {{allow-force-withdrawal-on-savings-account}}:
* {{enabled}}: whether force withdrawals are allowed (unchanged).
* {{value}}: the maximum negative balance allowed (same meaning and unit as the
current limit).
and remove {{force-withdrawal-on-savings-account-limit}}.
Migration (Liquibase, portable across PostgreSQL and MariaDB):
* copy the value of the limit configuration into
{{allow-force-withdrawal-on-savings-account}};
* delete the limit configuration row.
For installations that already configured both rows, the effective behaviour
does not change.
h2. Code impact (as far as I can see)
* {{GlobalConfigurationConstants}}: remove
{{FORCE_WITHDRAWAL_ON_SAVINGS_ACCOUNT_LIMIT}}.
* {{ConfigurationDomainService}} / {{ConfigurationDomainServiceJpa}}: remove
{{retrieveForceWithdrawalOnSavingsAccountLimit()}} and read the value of the
remaining configuration.
* {{SavingsAccount#isForceWithdrawalAllowed}}: read the limit from the
remaining configuration.
* Integration tests and test helpers that list or set the two configurations.
h2. Open questions
# Since the feature shipped in 1.15.0, should the old configuration be removed
outright in 1.16.0, or deprecated for one release first?
# What should enabled=true with value=0 mean? Today, with a limit of 0, no
negative balance is allowed.
# Should the value be validated as non-negative?
# Does anyone depend on the current configuration names?
h2. Possible follow-up (out of scope for this change)
It would be useful to allow force withdrawals without any limit. I would keep
that separate, because the merge above can preserve the current behaviour
exactly, while an unlimited overdraft is a different risk decision. If it is
wanted, the way to represent "no limit" needs care:
* value=0 is not an option: it means "no negative balance" today and is the
default of both rows, so existing installations that enabled the feature
without setting a limit would silently become unlimited.
* A negative sentinel such as {{-1}} is not safe either: the current code
accepts negative values as valid limits (a negative value is used as is, a
positive one is negated), so {{-1}} already means a limit of 1.
* null (the column is nullable) would make "unset" mean "unlimited", which is
an unsafe default, and the current code would fail with a NullPointerException.
Whatever the representation, it should be an explicit setting that is never
reached by default or by the migration.
Related: FINERACT-2471 (introduced the feature), FINERACT-2797 and
FINERACT-2929 (force-withdrawal validation fixes).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
