You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "abraham-menyhart (via GitHub)" <gi...@apache.org> on 2023/06/06 10:03:04 UTC
[GitHub] [fineract] abraham-menyhart commented on a diff in pull request #3220: FINERACT-1806-Post-ChargeOff-Scenarios-1
abraham-menyhart commented on code in PR #3220:
URL: https://github.com/apache/fineract/pull/3220#discussion_r1219238103
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java:
##########
@@ -4484,6 +4486,147 @@ public BigDecimal getProposedPrincipal() {
return this.proposedPrincipal;
}
+ public List<Map<String, Object>> deriveAccountingBridgeDataForChargeOff(final String currencyCode,
Review Comment:
For readability, I would suggest to break this big method into smaller parts (10-20 lines/method).
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java:
##########
@@ -4484,6 +4486,147 @@ public BigDecimal getProposedPrincipal() {
return this.proposedPrincipal;
}
+ public List<Map<String, Object>> deriveAccountingBridgeDataForChargeOff(final String currencyCode,
+ final List<Long> existingTransactionIds, final List<Long> existingReversedTransactionIds, boolean isAccountTransfer) {
+
+ final List<Map<String, Object>> accountingBridgeData = new ArrayList<>();
+
+ final Map<String, Object> accountingBridgeDataGenericAttributes = new LinkedHashMap<>();
+ accountingBridgeDataGenericAttributes.put("loanId", getId());
+ accountingBridgeDataGenericAttributes.put("loanProductId", productId());
+ accountingBridgeDataGenericAttributes.put("officeId", getOfficeId());
+ accountingBridgeDataGenericAttributes.put("currencyCode", currencyCode);
+ accountingBridgeDataGenericAttributes.put("calculatedInterest", this.summary.getTotalInterestCharged());
+ accountingBridgeDataGenericAttributes.put("cashBasedAccountingEnabled", isCashBasedAccountingEnabledOnLoanProduct());
+ accountingBridgeDataGenericAttributes.put("upfrontAccrualBasedAccountingEnabled", isUpfrontAccrualAccountingEnabledOnLoanProduct());
+ accountingBridgeDataGenericAttributes.put("periodicAccrualBasedAccountingEnabled",
+ isPeriodicAccrualAccountingEnabledOnLoanProduct());
+ accountingBridgeDataGenericAttributes.put("isAccountTransfer", isAccountTransfer);
+
+ // get map before charge-off
+ final Map<String, Object> accountingBridgeDataBeforeChargeOff = new LinkedHashMap<>(accountingBridgeDataGenericAttributes);
Review Comment:
You added 3 comments here, in this method, they are perfect markers what you should extract into smaller methods.
Like this first one, you commented "get map before charge-off", and this could be extracted into getAccountingBridgeDataBeforeChargeOff(), or something like this.
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java:
##########
@@ -4484,6 +4486,147 @@ public BigDecimal getProposedPrincipal() {
return this.proposedPrincipal;
}
+ public List<Map<String, Object>> deriveAccountingBridgeDataForChargeOff(final String currencyCode,
+ final List<Long> existingTransactionIds, final List<Long> existingReversedTransactionIds, boolean isAccountTransfer) {
+
+ final List<Map<String, Object>> accountingBridgeData = new ArrayList<>();
+
+ final Map<String, Object> accountingBridgeDataGenericAttributes = new LinkedHashMap<>();
Review Comment:
For example extract this part (creating the accountingBridgeDataGenericAttributes) into a method, like createAccountingBridgeDataGenericAttributes, or something like this.
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java:
##########
@@ -4484,6 +4486,147 @@ public BigDecimal getProposedPrincipal() {
return this.proposedPrincipal;
}
+ public List<Map<String, Object>> deriveAccountingBridgeDataForChargeOff(final String currencyCode,
+ final List<Long> existingTransactionIds, final List<Long> existingReversedTransactionIds, boolean isAccountTransfer) {
+
+ final List<Map<String, Object>> accountingBridgeData = new ArrayList<>();
+
+ final Map<String, Object> accountingBridgeDataGenericAttributes = new LinkedHashMap<>();
+ accountingBridgeDataGenericAttributes.put("loanId", getId());
+ accountingBridgeDataGenericAttributes.put("loanProductId", productId());
+ accountingBridgeDataGenericAttributes.put("officeId", getOfficeId());
+ accountingBridgeDataGenericAttributes.put("currencyCode", currencyCode);
+ accountingBridgeDataGenericAttributes.put("calculatedInterest", this.summary.getTotalInterestCharged());
+ accountingBridgeDataGenericAttributes.put("cashBasedAccountingEnabled", isCashBasedAccountingEnabledOnLoanProduct());
+ accountingBridgeDataGenericAttributes.put("upfrontAccrualBasedAccountingEnabled", isUpfrontAccrualAccountingEnabledOnLoanProduct());
+ accountingBridgeDataGenericAttributes.put("periodicAccrualBasedAccountingEnabled",
+ isPeriodicAccrualAccountingEnabledOnLoanProduct());
+ accountingBridgeDataGenericAttributes.put("isAccountTransfer", isAccountTransfer);
+
+ // get map before charge-off
+ final Map<String, Object> accountingBridgeDataBeforeChargeOff = new LinkedHashMap<>(accountingBridgeDataGenericAttributes);
+ accountingBridgeDataBeforeChargeOff.put("isChargeOff", false);
+ accountingBridgeDataBeforeChargeOff.put("isFraud", false);
+
+ Predicate<LoanTransaction> isBeforeChargeOff = transaction -> transaction.getTransactionDate().isBefore(getChargedOffOnDate());
+ final List<Map<String, Object>> newLoanTransactionsBeforeChargeOff = new ArrayList<>();
+
+ getTransactionsForAccountingBridgeData(currencyCode, existingTransactionIds, existingReversedTransactionIds,
+ newLoanTransactionsBeforeChargeOff, isBeforeChargeOff);
+
+ // get map after charge-off
+ final Map<String, Object> accountingBridgeDataAfterChargeOff = new LinkedHashMap<>(accountingBridgeDataGenericAttributes);
+ accountingBridgeDataAfterChargeOff.put("isChargeOff", isChargedOff());
+ accountingBridgeDataAfterChargeOff.put("isFraud", isFraud());
+
+ Predicate<LoanTransaction> isAfterChargeOff = transaction -> transaction.getTransactionDate().isAfter(getChargedOffOnDate());
+ final List<Map<String, Object>> newLoanTransactionsAfterChargeOff = new ArrayList<>();
+
+ getTransactionsForAccountingBridgeData(currencyCode, existingTransactionIds, existingReversedTransactionIds,
+ newLoanTransactionsAfterChargeOff, isAfterChargeOff);
+
+ // get map onCharge off date
+ Predicate<LoanTransaction> isOnChargeOff = transaction -> transaction.getTransactionDate().isEqual(getChargedOffOnDate());
+ getTransactionsForAccountingBridgeDataOnChargeOffDate(currencyCode, existingTransactionIds, existingReversedTransactionIds,
+ newLoanTransactionsBeforeChargeOff, newLoanTransactionsAfterChargeOff, isOnChargeOff);
+
+ accountingBridgeDataBeforeChargeOff.put("newLoanTransactions", newLoanTransactionsBeforeChargeOff);
+ accountingBridgeData.add(accountingBridgeDataBeforeChargeOff);
+
+ accountingBridgeDataAfterChargeOff.put("newLoanTransactions", newLoanTransactionsAfterChargeOff);
+ accountingBridgeData.add(accountingBridgeDataAfterChargeOff);
+
+ return accountingBridgeData;
+ }
+
+ private void getTransactionsForAccountingBridgeDataOnChargeOffDate(String currencyCode, List<Long> existingTransactionIds,
+ List<Long> existingReversedTransactionIds, List<Map<String, Object>> newLoanTransactionsBeforeChargeOff,
+ List<Map<String, Object>> newLoanTransactionsAfterChargeOff, Predicate<LoanTransaction> chargeOffDateCriteria) {
+ List<LoanTransaction> transactionsOnChargeOffDate = this.loanTransactions.stream().filter(chargeOffDateCriteria)
+ .collect(Collectors.toList());
+ /**
+ *
+ * TODO: Modify logic to retrieve correct charge-off transaction once reverse replay of charge-off is
+ * implemented
+ */
+ LoanTransaction chargeOffTransaction = this.loanTransactions.stream().filter(LoanTransaction::isChargeOff).findFirst().get();
+ Predicate<LoanTransactionRelation> isReplayed = transactionRelation -> transactionRelation.getRelationType()
+ .equals(LoanTransactionRelationTypeEnum.REPLAYED);
+ for (final LoanTransaction transaction : transactionsOnChargeOffDate) {
+ if (!transaction.isChargeOff()) {
+ if (transaction.isReversed() && existingTransactionIds.contains(transaction.getId())
+ && !existingReversedTransactionIds.contains(transaction.getId())) {
+ // transaction on charge-off date got reversed
+ compareWithChargeOffIdAndAddTransactionForAccountingData(currencyCode, newLoanTransactionsBeforeChargeOff,
+ newLoanTransactionsAfterChargeOff, transaction, chargeOffTransaction.getId());
+
+ } else if (!existingTransactionIds.contains(transaction.getId())) {
+ // new transaction,backdated transaction or reverse replay on chargeoff date
+
+ // check transaction relations
+ if (!transaction.getLoanTransactionRelations().isEmpty()) {
+ // if relations has REPLAYED relation then it is reverse replay scenario
+ List<LoanTransactionRelation> replayedTransactionRelations = transaction.getLoanTransactionRelations().stream()
+ .filter(isReplayed).collect(Collectors.toList());
+ if (!replayedTransactionRelations.isEmpty()) {
+ // get transaction Id for first reversed transaction relation
+ Long transactionIdForReversedTransaction = replayedTransactionRelations.get(0).getToTransaction().getId();
+ if (transactionIdForReversedTransaction > chargeOffTransaction.getId()) {
Review Comment:
I'm not sure about this, but are we using transaction id here, to determine whether the transaction happened before/after the chargeOff? I saw transactionDate in the code above, I think that is a better approach to use that.
But I'm not familiar with the business logic here. Maybe @adamsaghy could help me with this.
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java:
##########
@@ -4484,6 +4486,147 @@ public BigDecimal getProposedPrincipal() {
return this.proposedPrincipal;
}
+ public List<Map<String, Object>> deriveAccountingBridgeDataForChargeOff(final String currencyCode,
+ final List<Long> existingTransactionIds, final List<Long> existingReversedTransactionIds, boolean isAccountTransfer) {
+
+ final List<Map<String, Object>> accountingBridgeData = new ArrayList<>();
+
+ final Map<String, Object> accountingBridgeDataGenericAttributes = new LinkedHashMap<>();
+ accountingBridgeDataGenericAttributes.put("loanId", getId());
+ accountingBridgeDataGenericAttributes.put("loanProductId", productId());
+ accountingBridgeDataGenericAttributes.put("officeId", getOfficeId());
+ accountingBridgeDataGenericAttributes.put("currencyCode", currencyCode);
+ accountingBridgeDataGenericAttributes.put("calculatedInterest", this.summary.getTotalInterestCharged());
+ accountingBridgeDataGenericAttributes.put("cashBasedAccountingEnabled", isCashBasedAccountingEnabledOnLoanProduct());
+ accountingBridgeDataGenericAttributes.put("upfrontAccrualBasedAccountingEnabled", isUpfrontAccrualAccountingEnabledOnLoanProduct());
+ accountingBridgeDataGenericAttributes.put("periodicAccrualBasedAccountingEnabled",
+ isPeriodicAccrualAccountingEnabledOnLoanProduct());
+ accountingBridgeDataGenericAttributes.put("isAccountTransfer", isAccountTransfer);
+
+ // get map before charge-off
+ final Map<String, Object> accountingBridgeDataBeforeChargeOff = new LinkedHashMap<>(accountingBridgeDataGenericAttributes);
+ accountingBridgeDataBeforeChargeOff.put("isChargeOff", false);
+ accountingBridgeDataBeforeChargeOff.put("isFraud", false);
+
+ Predicate<LoanTransaction> isBeforeChargeOff = transaction -> transaction.getTransactionDate().isBefore(getChargedOffOnDate());
+ final List<Map<String, Object>> newLoanTransactionsBeforeChargeOff = new ArrayList<>();
+
+ getTransactionsForAccountingBridgeData(currencyCode, existingTransactionIds, existingReversedTransactionIds,
+ newLoanTransactionsBeforeChargeOff, isBeforeChargeOff);
+
+ // get map after charge-off
+ final Map<String, Object> accountingBridgeDataAfterChargeOff = new LinkedHashMap<>(accountingBridgeDataGenericAttributes);
+ accountingBridgeDataAfterChargeOff.put("isChargeOff", isChargedOff());
+ accountingBridgeDataAfterChargeOff.put("isFraud", isFraud());
+
+ Predicate<LoanTransaction> isAfterChargeOff = transaction -> transaction.getTransactionDate().isAfter(getChargedOffOnDate());
+ final List<Map<String, Object>> newLoanTransactionsAfterChargeOff = new ArrayList<>();
+
+ getTransactionsForAccountingBridgeData(currencyCode, existingTransactionIds, existingReversedTransactionIds,
+ newLoanTransactionsAfterChargeOff, isAfterChargeOff);
+
+ // get map onCharge off date
+ Predicate<LoanTransaction> isOnChargeOff = transaction -> transaction.getTransactionDate().isEqual(getChargedOffOnDate());
+ getTransactionsForAccountingBridgeDataOnChargeOffDate(currencyCode, existingTransactionIds, existingReversedTransactionIds,
+ newLoanTransactionsBeforeChargeOff, newLoanTransactionsAfterChargeOff, isOnChargeOff);
+
+ accountingBridgeDataBeforeChargeOff.put("newLoanTransactions", newLoanTransactionsBeforeChargeOff);
+ accountingBridgeData.add(accountingBridgeDataBeforeChargeOff);
+
+ accountingBridgeDataAfterChargeOff.put("newLoanTransactions", newLoanTransactionsAfterChargeOff);
+ accountingBridgeData.add(accountingBridgeDataAfterChargeOff);
+
+ return accountingBridgeData;
+ }
+
+ private void getTransactionsForAccountingBridgeDataOnChargeOffDate(String currencyCode, List<Long> existingTransactionIds,
+ List<Long> existingReversedTransactionIds, List<Map<String, Object>> newLoanTransactionsBeforeChargeOff,
+ List<Map<String, Object>> newLoanTransactionsAfterChargeOff, Predicate<LoanTransaction> chargeOffDateCriteria) {
+ List<LoanTransaction> transactionsOnChargeOffDate = this.loanTransactions.stream().filter(chargeOffDateCriteria)
+ .collect(Collectors.toList());
+ /**
+ *
+ * TODO: Modify logic to retrieve correct charge-off transaction once reverse replay of charge-off is
+ * implemented
+ */
+ LoanTransaction chargeOffTransaction = this.loanTransactions.stream().filter(LoanTransaction::isChargeOff).findFirst().get();
+ Predicate<LoanTransactionRelation> isReplayed = transactionRelation -> transactionRelation.getRelationType()
Review Comment:
Generally, it is better to use the `equals` on the constant, not on the object. If the `getRelationType()` returns a null, then you got `NullPointException`. But if you wrote `REPLAYED.equals(transactionRelation.getRelationType())` , then you are safe. Or you could use `Objects.equals()`, and the order wont matter.
--
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