adamsaghy commented on code in PR #6585:
URL: https://github.com/apache/fineract/pull/6585#discussion_r4308318206
##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/ProgressiveLoanTransactionValidatorImpl.java:
##########
@@ -267,59 +300,83 @@ public void validateContractTermination(final JsonCommand
command, final Long lo
return;
}
- Validator.validateOrThrow("loan.contract.termination",
baseDataValidator -> {
+ Validator.validateOrThrow(rules.resource(), baseDataValidator -> {
if (transactionDate == null) {
// A sent but unparseable date reads as null too, and must not
take the business date default
baseDataValidator.reset().parameter(TRANSACTION_DATE).failWithCode("invalid.date.format",
- "Contract termination transaction date could not be
read as a date");
+ rules.label() + " transaction date could not be read
as a date");
} else if (DateUtils.isBeforeBusinessDate(transactionDate)) {
-
baseDataValidator.reset().parameter(TRANSACTION_DATE).value(transactionDate).failWithCode("cannot.be.before.business.date",
- "Contract termination transaction date cannot be
before the business date");
+ if (rules.backdatingAllowed()) {
+ validateBackdatedEarlyTermination(baseDataValidator,
transactionDate,
+
this.loanRepositoryWrapper.findOneWithNotFoundDetection(loanId, true), rules);
+ } else {
+
baseDataValidator.reset().parameter(TRANSACTION_DATE).value(transactionDate).failWithCode(
+ "cannot.be.before.business.date", rules.label() +
" transaction date cannot be before the business date");
+ }
} else if (DateUtils.isDateInTheFuture(transactionDate)) {
// Deliberately future dated only: terminating on the business
date after maturity stays allowed
final Loan loan =
this.loanRepositoryWrapper.findOneWithNotFoundDetection(loanId);
final LocalDate maturityDate = loan.getMaturityDate();
if (maturityDate != null &&
!transactionDate.isBefore(maturityDate)) {
baseDataValidator.reset().parameter(TRANSACTION_DATE).value(transactionDate).failWithCode(
- "must.be.before.maturity.date", "Contract
termination transaction date must be before the maturity date");
+ "must.be.before.maturity.date", rules.label() + "
transaction date must be before the maturity date");
}
}
});
}
- @Override
- public void validateContractTerminationUndo(final JsonCommand command,
final Long loanId) {
+ /**
+ * A backdated early termination may not move behind anything already
recorded on the loan: its last transaction and
+ * the charges submitted on it.
+ */
+ private void validateBackdatedEarlyTermination(final DataValidatorBuilder
baseDataValidator, final LocalDate transactionDate,
+ final Loan loan, final EarlyTerminationRules rules) {
+ if (DateUtils.isBefore(transactionDate,
loan.getLastUserTransactionDate())) {
Review Comment:
**Change request:** `Loan#getLastUserTransactionDate()` only excludes
accrual, accrual adjustment, accrual activity and income posting. Capitalized
income amortization (+ adjustment) and buy-down fee amortization (+ adjustment)
count as "user" transactions here. COB posts these every day, dated the COB
date (`processCapitalizedIncomeAmortizationTillDate(loan, businessDate, ...)`),
so on any loan with capitalized income or a buy-down fee, every withdrawal
dated before yesterday is rejected with
`cannot.be.before.last.transaction.date`.
The AC says "last **monetary** transaction". Please exclude these
non-monetary amortization types. The exclusion list in
`Loan#getLastUserTransaction()`, which the undo check already uses, would also
keep apply and undo consistent. Please also add an IT for a backdated
withdrawal on a loan with capitalized income.
##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/transactionprocessor/impl/AdvancedPaymentScheduleTransactionProcessor.java:
##########
@@ -2763,6 +2753,8 @@ private Money
processAllocationsHorizontally(LoanTransaction loanTransaction, Tr
paymentAllocationContext.setInAdvanceInstallmentsFilteringRules(
installment ->
loanTransaction.isBefore(installment.getDueDate()) &&
installment.isNotFullyPaidOff());
}
+ // An early termination accelerates what it collapses onto its date,
so on that date it is already past due
+ final boolean terminatedEarly =
loanTransaction.getLoan().isTerminatedEarlyOnDate(loanTransaction.getTransactionDate());
Review Comment:
**Question:** this treats the accelerated installment as past due on the
termination date, but only in the horizontal path. `processPeriodsVertically`
still selects it in the `DUE` branch (`isOn(e.getDueDate())`, ~L3128) and the
`PAST_DUE` branch still requires `isAfter`. A same-day repayment can therefore
allocate differently depending on the strategy. For example, with
`PAST_DUE_INTEREST, …, DUE_PRINCIPAL, DUE_INTEREST` the vertical path pays
principal before interest, the opposite of what the new "repayment on the
withdrawal date pays the interest due before the principal" scenario asserts
for horizontal. Is that intended? If not, please apply the same rule in the
vertical path and add a vertical scenario. This also applies to contract
termination, not just withdrawal.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/contracttermination/LoanContractTerminationServiceImpl.java:
##########
@@ -128,39 +160,33 @@ public CommandProcessingResult
applyContractTermination(final JsonCommand comman
.build();
}
- public CommandProcessingResult undoContractTermination(final JsonCommand
command) {
+ private CommandProcessingResult undoTermination(final JsonCommand command,
final TerminationKind kind) {
final Long loanId = command.getLoanId();
-
- loanTransactionValidator.validateContractTerminationUndo(command,
loanId);
-
final Loan loan = loanAssembler.assembleFrom(loanId);
- final LoanTransaction contractTerminationTransaction =
loan.findContractTerminationTransaction();
+ final LoanTransaction termination = loan.getLoanTransaction(t ->
t.isNotReversed() && kind.transactionType().equals(t.getTypeOf()));
Review Comment:
Nit: this inline predicate duplicates
`Loan#findContractTerminationTransaction` /
`Loan#findLoanWithdrawalTransaction`, which the validator already uses through
`EarlyTerminationRules#transactionFinder`. Consider adding the finder to
`TerminationKind` so that apply, undo and validation look up the transaction
the same way.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/api/LoanTransactionsApiResource.java:
##########
@@ -686,6 +686,11 @@ private String retrieveTransactionTemplate(Long loanId,
String loanExternalIdStr
}
transactionData =
this.loanReadPlatformService.retrieveLoanPrePaymentTemplate(LoanTransactionType.REPAYMENT,
resolvedLoanId,
transactionDate);
+ } else if (CommandParameterUtil.is(commandParam,
LoanApiConstants.CONTRACT_TERMINATION_COMMAND)
+ || CommandParameterUtil.is(commandParam,
LoanApiConstants.LOAN_WITHDRAWAL_COMMAND)) {
Review Comment:
**Question:** `contractTermination` previously used the prepay template. It
now goes through `fetchEarlyTerminationDetail`, so the CT preview changes for
loans without interest recalculation. I assume that's a deliberate fix. Can you
confirm? Related: for non-recalc loans the processor calls
`recalculateInstallmentFeeCharges` before it sums outstanding amounts, while
`fetchEarlyTerminationDetail` sums the raw fee outstanding. Does the preview
still match the posted amount when installment-fee charges are on the loan? The
preview test only covers a loan without charges.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanTransactionProcessingServiceImpl.java:
##########
@@ -173,6 +173,36 @@ public OutstandingAmountsDTO fetchPrepaymentDetail(final
ScheduleGeneratorDTO sc
return outstandingAmounts;
}
+ @Override
+ public OutstandingAmountsDTO fetchEarlyTerminationDetail(final
ScheduleGeneratorDTO scheduleGeneratorDTO, final LocalDate onDate,
+ final Loan loan) {
+ final LoanRepaymentScheduleInstallment currentInstallment =
loan.getRelatedRepaymentScheduleInstallment(onDate);
+ if (loan.isInterestBearingAndInterestRecalculationEnabled() ||
currentInstallment == null
+ || !DateUtils.isBefore(onDate, loan.getMaturityDate())) {
+ return fetchPrepaymentDetail(scheduleGeneratorDTO, onDate, loan);
+ }
+ // Without interest recalculation the termination prorates the
interest of the installment it lands in and drops
Review Comment:
Nit: this comment's line wrap is broken (`drops` / `the` / `interest`).
Please join it back into two lines.
--
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]
