[
https://issues.apache.org/jira/browse/FINERACT-2934?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18126821#comment-18126821
]
Adam Saghy commented on FINERACT-2934:
--------------------------------------
[~nemux] [~ashutoshpanda] Let you two discuss this ;)
> 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
> Priority: Minor
>
> 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)
