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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
