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 07:55:57 UTC
[GitHub] [fineract] galovics commented on a change in pull request #2220: FINERACT-130:HoldAccount
galovics commented on a change in pull request #2220:
URL: https://github.com/apache/fineract/pull/2220#discussion_r839271074
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsAccountTransactionDataValidator.java
##########
@@ -179,6 +183,11 @@ public void validateClosing(final JsonCommand command, final SavingsAccount acco
account.getId());
}
+ if (account.getSubStatus().equals(400) || account.getSubStatus().equals(500) || account.getSubStatus().equals(600)) {
Review comment:
I know probably these magic numbers are all over the code but could we start extracting them?
If I just look at the code, 400, 500, 600 doesn't represent anything meaningful for me.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsAccountWritePlatformServiceJpaRepositoryImpl.java
##########
@@ -1934,4 +1965,10 @@ private void validateTransactionsForTransfer(final SavingsAccount savingsAccount
}
}
+
+ private void validateReasonForHold(String reasonForBlock) {
+ if ("".equals(reasonForBlock)) {
Review comment:
StringUtils.isBlank please.
##########
File path: fineract-provider/src/main/resources/db/changelog/tenant/parts/0009_hold_reason_savings_account.xml
##########
@@ -0,0 +1,57 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.1.xsd">
+ <changeSet author="fineract" id="1">
+ <addColumn tableName="m_savings_account">
+ <column name="reason_for_block" type="VARCHAR(50)"/>
Review comment:
Are you sure 50 characters will be enough here? I'd rather go with a tiny bit bigger like 256.
##########
File path: fineract-provider/src/main/resources/db/changelog/tenant/parts/0009_hold_reason_savings_account.xml
##########
@@ -0,0 +1,57 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.1.xsd">
+ <changeSet author="fineract" id="1">
+ <addColumn tableName="m_savings_account">
+ <column name="reason_for_block" type="VARCHAR(50)"/>
+ </addColumn>
+ </changeSet>
+ <changeSet author="fineract" id="2">
+ <addColumn tableName="m_savings_account_transaction">
+ <column name="reason_for_block" type="VARCHAR(50)"/>
+ </addColumn>
+ </changeSet>
+ <changeSet author="fineract" id="3">
+ <insert tableName="m_code">
+ <column name="id" valueNumeric="35"/>
Review comment:
Is the ID of the code important? Can't we just rely on the database generating them?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsAccountTransactionDataValidator.java
##########
@@ -225,28 +234,39 @@ public SavingsAccountTransaction validateHoldAndAssembleForm(final String json,
baseDataValidator.reset().parameter(transactionAmountParamName).value(amount).notNull().positiveAmount();
final LocalDate transactionDate = this.fromApiJsonHelper.extractLocalDateNamed(transactionDateParamName, element);
+ final String reasonForBlock = this.fromApiJsonHelper.extractStringNamed(SavingsApiConstants.reasonForBlockParamName, element);
+ baseDataValidator.reset().parameter(SavingsApiConstants.reasonForBlockParamName).value(reasonForBlock).notBlank()
+ .notExceedingLengthOf(100);
+
baseDataValidator.reset().parameter(transactionDateParamName).value(transactionDate).notNull();
boolean isActive = account.isActive();
if (!isActive) {
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());
- }
+ Money runningBalance = Money.of(account.getCurrency(), account.getAccountBalance());
+
+ if (account.getSavingsHoldAmount() != null) {
+ runningBalance = runningBalance.minus(account.getSavingsHoldAmount()).minus(amount);
+ } else {
+ runningBalance = runningBalance.minus(amount);
}
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());
+ }
+
+ account.holdAmount(amount);
Review comment:
Wait a second, isn't this a Validator class? Why is in doing anything with the state of the SavingsAccount? It definitely shoudn't introduce side effects..
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsAccountTransactionDataValidator.java
##########
@@ -271,6 +291,10 @@ public SavingsAccountTransaction validateHoldAndAssembleForm(final String json,
SavingsAccountTransaction transaction = SavingsAccountTransaction.holdAmount(account, account.office(), paymentDetails,
transactionDate, Money.of(account.getCurrency(), amount), createdDate, createdUser);
+
Review comment:
Generally speaking I don't like that a validator is actually creating new domain objects like a SavingsAccountTransaction. Seems like we have given the validator way too much responsibility and I'd prefer to split it up.
I see it wasn't this PR that introduced this approach so I won't block it because of this but would be really beneficial to divide these.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountTransaction.java
##########
@@ -127,6 +127,9 @@
@Column(name = "release_id_of_hold_amount", length = 20)
private Long releaseIdOfHoldAmountTransaction;
+ @Column(name = "reason_for_block", nullable = true)
Review comment:
Wait, why do we need a field like reasaonForBlock on the transaction as well?
According to the JIRA ticket, only an account level block should exist. What am I missing?
--
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