Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FINERACT-2156: Confirm existance of elements before accessing them #4202

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+10 −3
Open

FINERACT-2156: Confirm existance of elements before accessing them #4202

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions ...ct-savings/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccount.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1594,12 +1594,19 @@ public void validateAccountBalanceDoesNotBecomeNegative(final String transaction
public void validateAccountBalanceDoesNotViolateOverdraft(final List<SavingsAccountTransaction> savingsAccountTransaction,
final BigDecimal amountPaid) {
if (savingsAccountTransaction != null) {
SavingsAccountTransaction savingsAccountTransactionFirst = savingsAccountTransaction.get(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind to simplify the logic a little bit further?

You can merge the savingsAccountTransaction != null and savingsAccountTransaction.size() > 0 condition together and if they are TRUE take the running balance of the first element otherwise it is BigDecimal.ZERO (if there are no any transaction the running balance must be 0).
After you can deduct the amountPaid from the above balance and if the balance is negative and allowOverdraft is FALSE then InsufficientAccountBalanceException to be thrown.

I believe it would be easier to read and cover better the requirements!

Also kindly asking you to write a test (it can be unit tests), which covers the following situations:

  • savingsAccountTransaction is empty and amountPaid is positive and allowOverdraft = true -> No exception!
  • savingsAccountTransaction is empty and amountPaid is positive and allowOverdraft = false -> Exception to be thrown!
  • savingsAccountTransaction is not empty and amountPaid is positive and amountPaid is higher than the running balance of the 1st transaciton and allowOverdraft = true -> No exception!
  • savingsAccountTransaction is not empty and amountPaid is positive and amountPaid is lower than the running balance of the 1st transaction and allowOverdraft = false -> Exception to be thrown!
  • savingsAccountTransaction is not empty and amountPaid is positive and amountPaid is equal with the running balance of the 1st transaction and allowOverdraft = false -> No exception!

if (!this.allowOverdraft) {
if (savingsAccountTransactionFirst.getRunningBalance(this.currency).minus(amountPaid).isLessThanZero()) {
if (savingsAccountTransaction.size() > 0) {
SavingsAccountTransaction savingsAccountTransactionFirst = savingsAccountTransaction.get(0);
if (!this.allowOverdraft) {
if (savingsAccountTransactionFirst.getRunningBalance(this.currency).minus(amountPaid).isLessThanZero()) {
throw new InsufficientAccountBalanceException("transactionAmount", getAccountBalance(), null, amountPaid);
}
}
} else {
if (!this.allowOverdraft) {
throw new InsufficientAccountBalanceException("transactionAmount", getAccountBalance(), null, amountPaid);
}
}

}
}

Expand Down
Loading