You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2022/03/31 08:11:23 UTC
[GitHub] [fineract] galovics commented on a change in pull request #2215: FINERACT-1553: Lien configured at product level
galovics commented on a change in pull request #2215:
URL: https://github.com/apache/fineract/pull/2215#discussion_r839296376
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsAccountTransactionDataValidator.java
##########
@@ -232,25 +232,68 @@ public SavingsAccountTransaction validateHoldAndAssembleForm(final String json,
baseDataValidator.reset().parameter(SavingsApiConstants.statusParamName)
.failWithCodeNoParameterAddedToErrorCode(SavingsApiConstants.ERROR_MSG_SAVINGS_ACCOUNT_NOT_ACTIVE);
}
- account.holdAmount(amount);
- if (account.getEnforceMinRequiredBalance()) {
- if (account.getWithdrawableBalance().compareTo(BigDecimal.ZERO) < 0) {
- baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient balance", account.getId());
+ Boolean isEnforceMinRequiredBalanceEnabled = account.getEnforceMinRequiredBalance();
+ Boolean isAccountLienEnabled = account.isLienAllowed();
+ Boolean isOverdraftEnabled = account.isAllowOverdraft();
+
+ Boolean lienAllowed = false;
Review comment:
This could be simplified into
`BooleanUtils.isTrue(fromApiJsonHelper.extractBooleanNamed(lienAllowedParamName, element))`
instead of the multi-layered if statements.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsAccountTransactionDataValidator.java
##########
@@ -232,25 +232,68 @@ public SavingsAccountTransaction validateHoldAndAssembleForm(final String json,
baseDataValidator.reset().parameter(SavingsApiConstants.statusParamName)
.failWithCodeNoParameterAddedToErrorCode(SavingsApiConstants.ERROR_MSG_SAVINGS_ACCOUNT_NOT_ACTIVE);
}
- account.holdAmount(amount);
- if (account.getEnforceMinRequiredBalance()) {
- if (account.getWithdrawableBalance().compareTo(BigDecimal.ZERO) < 0) {
- baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient balance", account.getId());
+ Boolean isEnforceMinRequiredBalanceEnabled = account.getEnforceMinRequiredBalance();
+ Boolean isAccountLienEnabled = account.isLienAllowed();
+ Boolean isOverdraftEnabled = account.isAllowOverdraft();
+
+ Boolean lienAllowed = false;
+ if (this.fromApiJsonHelper.parameterExists(lienAllowedParamName, element)) {
+ lienAllowed = this.fromApiJsonHelper.extractBooleanNamed(lienAllowedParamName, element);
+ if (lienAllowed) {
+ if (isAccountLienEnabled) {
+ if (isOverdraftEnabled) {
+ if (account.getOverdraftLimit().compareTo(BigDecimal.ZERO) > 0
+ && account.getMaxAllowedLienLimit().compareTo(BigDecimal.ZERO) > 0) {
+ if (account.getOverdraftLimit().compareTo(account.getMaxAllowedLienLimit()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode(
+ "Overdraft.limit.can.not.be.greater.than.lien.limit", account.getId());
+ }
+ }
+ }
+
+ if (amount.compareTo(account.getMaxAllowedLienLimit()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("lien.limit.exceeded", account.getId());
+ }
+ } else {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("lien.is.not.allowed.in.product.level",
+ account.getId());
+ }
+ } else {
+ if (isOverdraftEnabled) {
+ if (amount.compareTo(account.getWithdrawableBalance()) > 0 && amount.compareTo(account.getOverdraftLimit()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient.balance", account.getId());
+ }
+ }
+ if (isEnforceMinRequiredBalanceEnabled) {
+ if (amount.compareTo(account.getWithdrawableBalance()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient.balance", account.getId());
+ }
+ }
+ if (!isOverdraftEnabled && !isEnforceMinRequiredBalanceEnabled) {
+ if (amount.compareTo(account.getWithdrawableBalance()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient.balance", account.getId());
+ }
+ }
}
- }
-
- Boolean lien = false;
-
- if (this.fromApiJsonHelper.parameterExists(lienParamName, element)) {
- lien = this.fromApiJsonHelper.extractBooleanNamed(lienParamName, element);
- if (!lien) {
- if (account.getWithdrawableBalanceWithoutMinimumBalance().compareTo(BigDecimal.ZERO) < 0) {
- baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient balance", account.getId());
+ } else {
+ if (isOverdraftEnabled) {
+ if (amount.compareTo(account.getWithdrawableBalance()) > 0 && amount.compareTo(account.getOverdraftLimit()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient.balance", account.getId());
+ }
+ }
+ if (isEnforceMinRequiredBalanceEnabled) {
+ if (amount.compareTo(account.getWithdrawableBalance()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient.balance", account.getId());
+ }
+ }
+ if (!isOverdraftEnabled && !isEnforceMinRequiredBalanceEnabled) {
+ if (amount.compareTo(account.getWithdrawableBalance()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient.balance", account.getId());
}
}
}
-
+ account.holdAmount(amount);
Review comment:
I commented this on another PR but I really think we should avoid things like this. A validator should not touch a domain object's state especially when it's a JPA entity.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsProductDataValidator.java
##########
@@ -340,6 +343,22 @@ public void validateForCreate(final String json) {
baseDataValidator.reset().parameter(minBalanceForInterestCalculationParamName).value(minBalanceForInterestCalculation)
.ignoreIfNull().zeroOrPositiveAmount();
}
+ Boolean isLienAllowed = this.fromApiJsonHelper.parameterExists(lienAllowedParamName, element);
+ Boolean isOverdraftAllowed = this.fromApiJsonHelper.parameterExists(allowOverdraftParamName, element);
Review comment:
Same as above.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccount.java
##########
@@ -290,6 +292,12 @@
@Column(name = "min_required_balance", scale = 6, precision = 19, nullable = true)
private BigDecimal minRequiredBalance;
+ @Column(name = "is_lien_allowed")
+ private boolean lienAllowed;
Review comment:
No need to. If `lienAllowed` is a binary switch then let's stick with primitive boolean. Less headache when dealing with it's value. Just make sure the database column is not nullable.
##########
File path: integration-tests/src/test/java/org/apache/fineract/integrationtests/ClientSavingsIntegrationTest.java
##########
@@ -2017,6 +2017,68 @@ private Integer createSavingsProduct(final RequestSpecification requestSpec, fin
enforceMinRequiredBalance, allowOverdraft, taxGroupId, false);
}
+ // LienAtProductLevel
+ private Integer createSavingsProduct(final RequestSpecification requestSpec, final ResponseSpecification responseSpec,
+ final String minOpenningBalance, String minBalanceForInterestCalculation, final boolean enforceMinRequiredBalance,
+ final boolean allowOverDraft, final boolean lienAllowed) {
+
+ LOG.info("------------------------------CREATING NEW SAVINGS PRODUCT WITH LIEN---------------------------------------");
+ SavingsProductHelper savingsProductHelper = new SavingsProductHelper();
+ if (lienAllowed) {
+ final String maxAllowedLienLimit = "2000.0";
+ savingsProductHelper.withLienAllowed(maxAllowedLienLimit);
+ }
+ if (enforceMinRequiredBalance) {
+ final String minRequiredBalance = "100.0";
+ savingsProductHelper.withMinRequiredBalance(minRequiredBalance);
+ savingsProductHelper.withEnforceMinRequiredBalance("true");
+ }
+ if (allowOverDraft) {
+ final String overDraftLimit = "1000.0";
+ savingsProductHelper.withOverDraft(overDraftLimit);
+ }
+ final String savingsProductJSON = savingsProductHelper.withInterestCompoundingPeriodTypeAsDaily()
+ //
Review comment:
Why the slashes?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccount.java
##########
@@ -290,6 +292,12 @@
@Column(name = "min_required_balance", scale = 6, precision = 19, nullable = true)
private BigDecimal minRequiredBalance;
+ @Column(name = "is_lien_allowed")
+ private boolean lienAllowed;
+
+ @Column(name = "max_Allowed_Lien_Limit", scale = 6, precision = 19, nullable = true)
Review comment:
Use only lowercase characters in the colum name.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsAccountReadPlatformServiceImpl.java
##########
@@ -1504,7 +1517,9 @@ public SavingsAccountTransactionData mapRow(final ResultSet rs, @SuppressWarning
// ");
// sqlBuilder.append("sp.annual_fee_on_day as annualFeeOnDay ");
sqlBuilder.append("sp.min_required_balance as minRequiredBalance, ");
- sqlBuilder.append("sp.enforce_min_required_balance as enforceMinRequiredBalance ");
+ sqlBuilder.append("sp.enforce_min_required_balance as enforceMinRequiredBalance, ");
+ sqlBuilder.append("sp.max_Allowed_Lien_Limit as maxAllowedLienLimit, ");
Review comment:
Lowercase column name here as well.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsProductDataValidator.java
##########
@@ -340,6 +343,22 @@ public void validateForCreate(final String json) {
baseDataValidator.reset().parameter(minBalanceForInterestCalculationParamName).value(minBalanceForInterestCalculation)
.ignoreIfNull().zeroOrPositiveAmount();
}
+ Boolean isLienAllowed = this.fromApiJsonHelper.parameterExists(lienAllowedParamName, element);
Review comment:
I don't think this is correct because you're only checking whether the parameter exists instead of the parameter's value. Am I missing something?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsProductDataValidator.java
##########
@@ -340,6 +343,22 @@ public void validateForCreate(final String json) {
baseDataValidator.reset().parameter(minBalanceForInterestCalculationParamName).value(minBalanceForInterestCalculation)
.ignoreIfNull().zeroOrPositiveAmount();
}
+ Boolean isLienAllowed = this.fromApiJsonHelper.parameterExists(lienAllowedParamName, element);
Review comment:
I don't think this is correct because you're only checking whether the parameter exists instead of the parameter's value. Am I missing something?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsAccountReadPlatformServiceImpl.java
##########
@@ -332,6 +332,8 @@ public SavingsAccountData retrieveOne(final Long accountId) {
sqlBuilder.append("sa.min_balance_for_interest_calculation as minBalanceForInterestCalculation,");
sqlBuilder.append("sa.min_required_balance as minRequiredBalance, ");
sqlBuilder.append("sa.enforce_min_required_balance as enforceMinRequiredBalance, ");
+ sqlBuilder.append("sa.max_Allowed_Lien_Limit as maxAllowedLienLimit, ");
Review comment:
Lowercase colum names pls.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsAccountTransactionDataValidator.java
##########
@@ -232,25 +232,68 @@ public SavingsAccountTransaction validateHoldAndAssembleForm(final String json,
baseDataValidator.reset().parameter(SavingsApiConstants.statusParamName)
.failWithCodeNoParameterAddedToErrorCode(SavingsApiConstants.ERROR_MSG_SAVINGS_ACCOUNT_NOT_ACTIVE);
}
- account.holdAmount(amount);
- if (account.getEnforceMinRequiredBalance()) {
- if (account.getWithdrawableBalance().compareTo(BigDecimal.ZERO) < 0) {
- baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient balance", account.getId());
+ Boolean isEnforceMinRequiredBalanceEnabled = account.getEnforceMinRequiredBalance();
+ Boolean isAccountLienEnabled = account.isLienAllowed();
+ Boolean isOverdraftEnabled = account.isAllowOverdraft();
+
+ Boolean lienAllowed = false;
+ if (this.fromApiJsonHelper.parameterExists(lienAllowedParamName, element)) {
+ lienAllowed = this.fromApiJsonHelper.extractBooleanNamed(lienAllowedParamName, element);
+ if (lienAllowed) {
+ if (isAccountLienEnabled) {
+ if (isOverdraftEnabled) {
+ if (account.getOverdraftLimit().compareTo(BigDecimal.ZERO) > 0
+ && account.getMaxAllowedLienLimit().compareTo(BigDecimal.ZERO) > 0) {
+ if (account.getOverdraftLimit().compareTo(account.getMaxAllowedLienLimit()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode(
+ "Overdraft.limit.can.not.be.greater.than.lien.limit", account.getId());
+ }
+ }
+ }
+
+ if (amount.compareTo(account.getMaxAllowedLienLimit()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("lien.limit.exceeded", account.getId());
+ }
+ } else {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("lien.is.not.allowed.in.product.level",
+ account.getId());
+ }
+ } else {
+ if (isOverdraftEnabled) {
+ if (amount.compareTo(account.getWithdrawableBalance()) > 0 && amount.compareTo(account.getOverdraftLimit()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient.balance", account.getId());
+ }
+ }
+ if (isEnforceMinRequiredBalanceEnabled) {
+ if (amount.compareTo(account.getWithdrawableBalance()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient.balance", account.getId());
+ }
+ }
+ if (!isOverdraftEnabled && !isEnforceMinRequiredBalanceEnabled) {
+ if (amount.compareTo(account.getWithdrawableBalance()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient.balance", account.getId());
+ }
+ }
}
- }
-
- Boolean lien = false;
-
- if (this.fromApiJsonHelper.parameterExists(lienParamName, element)) {
- lien = this.fromApiJsonHelper.extractBooleanNamed(lienParamName, element);
- if (!lien) {
- if (account.getWithdrawableBalanceWithoutMinimumBalance().compareTo(BigDecimal.ZERO) < 0) {
- baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient balance", account.getId());
+ } else {
+ if (isOverdraftEnabled) {
+ if (amount.compareTo(account.getWithdrawableBalance()) > 0 && amount.compareTo(account.getOverdraftLimit()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient.balance", account.getId());
+ }
+ }
+ if (isEnforceMinRequiredBalanceEnabled) {
+ if (amount.compareTo(account.getWithdrawableBalance()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient.balance", account.getId());
+ }
+ }
+ if (!isOverdraftEnabled && !isEnforceMinRequiredBalanceEnabled) {
+ if (amount.compareTo(account.getWithdrawableBalance()) > 0) {
+ baseDataValidator.reset().failWithCodeNoParameterAddedToErrorCode("insufficient.balance", account.getId());
}
}
}
-
+ account.holdAmount(amount);
Review comment:
I commented this on another PR but I really think we should avoid things like this. A validator should not touch a domain object's state especially when it's a JPA entity.
##########
File path: integration-tests/src/test/java/org/apache/fineract/integrationtests/ClientSavingsIntegrationTest.java
##########
@@ -2017,6 +2017,68 @@ private Integer createSavingsProduct(final RequestSpecification requestSpec, fin
enforceMinRequiredBalance, allowOverdraft, taxGroupId, false);
}
+ // LienAtProductLevel
+ private Integer createSavingsProduct(final RequestSpecification requestSpec, final ResponseSpecification responseSpec,
+ final String minOpenningBalance, String minBalanceForInterestCalculation, final boolean enforceMinRequiredBalance,
+ final boolean allowOverDraft, final boolean lienAllowed) {
+
+ LOG.info("------------------------------CREATING NEW SAVINGS PRODUCT WITH LIEN---------------------------------------");
+ SavingsProductHelper savingsProductHelper = new SavingsProductHelper();
+ if (lienAllowed) {
+ final String maxAllowedLienLimit = "2000.0";
+ savingsProductHelper.withLienAllowed(maxAllowedLienLimit);
+ }
+ if (enforceMinRequiredBalance) {
+ final String minRequiredBalance = "100.0";
+ savingsProductHelper.withMinRequiredBalance(minRequiredBalance);
+ savingsProductHelper.withEnforceMinRequiredBalance("true");
+ }
+ if (allowOverDraft) {
+ final String overDraftLimit = "1000.0";
+ savingsProductHelper.withOverDraft(overDraftLimit);
+ }
+ final String savingsProductJSON = savingsProductHelper.withInterestCompoundingPeriodTypeAsDaily()
+ //
+ .withInterestPostingPeriodTypeAsMonthly()
+ //
+ .withInterestCalculationPeriodTypeAsDailyBalance()
+ //
+ .withMinBalanceForInterestCalculation(minBalanceForInterestCalculation)
+ //
+ .withMinimumOpenningBalance(minOpenningBalance).build();
+
+ return SavingsProductHelper.createSavingsProduct(savingsProductJSON, requestSpec, responseSpec);
+ }
+
+ // LienAtProductlevel with Overdraft Limit > Lien Limit
+ private Integer createSavingsProduct(final RequestSpecification requestSpec, final ResponseSpecification responseSpec,
+ final String minOpenningBalance, String minBalanceForInterestCalculation, final boolean enforceMinRequiredBalance,
+ final boolean allowOverDraft, final String overDraftLimit, final boolean lienAllowed, final String lienAllowedLimit) {
+ LOG.info("------------------------------CREATING NEW SAVINGS PRODUCT WITH LIEN---------------------------------------");
+ SavingsProductHelper savingsProductHelper = new SavingsProductHelper();
+ if (lienAllowed) {
+ savingsProductHelper.withLienAllowed(lienAllowedLimit);
+ }
+ if (enforceMinRequiredBalance) {
+ final String minRequiredBalance = "100.0";
+ savingsProductHelper.withMinRequiredBalance(minRequiredBalance);
+ savingsProductHelper.withEnforceMinRequiredBalance("true");
+ }
+ if (allowOverDraft) {
+ savingsProductHelper.withOverDraft(overDraftLimit);
+ }
+ final String savingsProductJSON = savingsProductHelper.withInterestCompoundingPeriodTypeAsDaily()
+ //
Review comment:
Slashes here too.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsProductDataValidator.java
##########
@@ -340,6 +343,22 @@ public void validateForCreate(final String json) {
baseDataValidator.reset().parameter(minBalanceForInterestCalculationParamName).value(minBalanceForInterestCalculation)
.ignoreIfNull().zeroOrPositiveAmount();
}
+ Boolean isLienAllowed = this.fromApiJsonHelper.parameterExists(lienAllowedParamName, element);
+ Boolean isOverdraftAllowed = this.fromApiJsonHelper.parameterExists(allowOverdraftParamName, element);
Review comment:
Same as above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org