FINERACT-2092: New transaction type interest payment waiver #3932

somasorosdpc · 2024-06-13T09:01:42Z

Description

introduce Payment Waiver transaction type
add integrtion tests
refactor code under the callstack of payment waiver command handler

Ignore if these details are present on the associated Apache Fineract JIRA ticket.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Write the commit message as per https://github.com/apache/fineract/#pull-requests
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

adamsaghy · 2024-06-14T10:57:44Z

...apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java

+
+    @Transactional
+    @Override
+    public CommandProcessingResult makeLoanRepaymentWithPaymentWaiver(final JsonCommand command) {


We might wanna rename this to makePaymentWaiver

adamsaghy · 2024-06-14T10:58:33Z

...apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java

+        return isChronologicallyLatestRepaymentOrWaiver;
+    }
+
+    LoanTransaction assembleTransactionAndCalculateChanges(Loan loan, JsonCommand command, Map<String, Object> changes) {


You might wanna move this into a LoanTransactionAssembler.

adamsaghy · 2024-06-14T11:01:29Z

...apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java

+                txnExternalId, null);
+    }
+
+    ChangedTransactionDetail assembleUpdatedLoan(Loan loan, LoanTransaction newPaymentWaiverTransaction) {


You might wanna rename this something more descriptive: example: recalculateLoanWithPaymentWaiverTxn

adamsaghy · 2024-06-14T11:06:27Z

...apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java

+        loan = saveAndFlushLoanWithDataIntegrityViolationChecks(loan);
+
+        // Save Note
+        if (StringUtils.isNotBlank(noteText)) {


Would it make sense to call the createNote(noteText, loan) instead?

adamsaghy · 2024-06-14T11:07:06Z

...apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java

+
+        // Save changedTransactionDetail if exists
+
+        if (changedTransactionDetail != null) {


Would it make sense to extract into a method this logic?

adamsaghy · 2024-06-14T11:08:41Z

...apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java

+        changes.put("paymentTypeId", command.longValueOfParameterNamed("paymentTypeId"));
+
+        if (StringUtils.isNotBlank(noteText)) {
+            changes.put("note", noteText);


Is there any logic which requires the note to be in the changes array?

We don't have any build-in logic on it. But as I see we send back it in the response, and other repayment type returns it, I think, we should keep it in the changes set to keep uniform response values.

adamsaghy · 2024-06-14T11:11:24Z

...apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java

+        doPostLoanTransactionChecks(loan, newPaymentWaiverTransaction.getTransactionDate(), defaultLoanLifecycleStateMachine);
+
+        // Payment allocation calculates the new total principal repaid and it should be validated after recalculation
+        if (loan.getLoanProduct().isMultiDisburseLoan()) {


Can we move this out from this method and be called after assembleTransactionAndCalculateChanges ?

adamsaghy · 2024-06-14T11:11:53Z

...apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java

+                }
+            }
+        }
+        if (reprocess) {


Would it make sense to extract it into a new method?

adamsaghy · 2024-06-14T11:12:50Z

...apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java

+        final LoanRepaymentScheduleInstallment currentInstallment = loan
+                .fetchLoanRepaymentScheduleInstallment(newPaymentWaiverTransaction.getTransactionDate());
+
+        boolean reprocess = true;


Would it make sense to extract it into a new method?

As I see, the logic calculates reprocess flag and also does repayment schedule calculation in some cases which strongly connects the main logic.

Maybe it would be better to move this where ever we storing the note. Thoughts?

adamsaghy

Please kindly check my review!

adamsaghy · 2024-06-19T09:13:54Z

fineract-core/src/main/java/org/apache/fineract/commands/service/CommandWrapperBuilder.java

+        this.entityName = "LOAN";
+        this.entityId = null;
+        this.loanId = loanId;
+        this.href = "/loans/" + loanId + "/transactions/template?command=paymentwaiver";


Please change this to be "interestPaymentWaiver"

adamsaghy · 2024-06-19T09:15:20Z

...g/apache/fineract/portfolio/loanaccount/handler/LoanInterestPaymentWaiverCommandHandler.java

+    public CommandProcessingResult processCommand(final JsonCommand command) {
+        try {
+            return this.writePlatformService.makeInterestPaymentWaiver(command);
+        } catch (final JpaSystemException | DataIntegrityViolationException dve) {


I think we can remove the try catch from here. Let it be handled in the error mappers ;)

adamsaghy · 2024-06-19T09:15:44Z

...n/java/org/apache/fineract/portfolio/loanaccount/serialization/LoanTransactionValidator.java

+        }
+    }
+
+    public void validateLoanStatusIsActiveOrFullyPAidOrOverpaid(final Loan loan) {


typo in the method name

adamsaghy · 2024-06-19T09:25:51Z

fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java

    }

-    private boolean isForeclosure() {
+    public Integer getLoanSubStatus() {


Lombok getter is on the Loan entity, so probably this getter method can be removed.

adamsaghy · 2024-06-19T09:40:07Z

...test/java/org/apache/fineract/integrationtests/LoanTransactionInterestPaymentWaiverTest.java

+                .addAdvancedPaymentAllocation(defaultAllocation, goodwillCreditAllocation, merchantIssuedRefundAllocation,
+                        payoutRefundAllocation, interestPaymentWaiver)
+                .withInterestCalculationPeriodTypeAsRepaymentPeriod(true).withInterestTypeAsDecliningBalance().withMultiDisburse()
+                .withDisallowExpectedDisbursements(true).withLoanScheduleType(loanScheduleType)


Please rewrite the test cases to the following:

Loans with interest

The test cases are checking the accounting entries are proper

adamsaghy · 2024-06-21T09:38:30Z

fineract-core/src/main/java/org/apache/fineract/commands/service/CommandWrapperBuilder.java

+        this.entityName = "LOAN";
+        this.entityId = null;
+        this.loanId = loanId;
+        this.href = "/loans/" + loanId + "/transactions/template?command=interestpaymentwaiver";


typo: interestpaymentwaiver -> interestPaymentWaiver

adamsaghy

LGTM

somasorosdpc changed the title ~~Fineract 2092/new transaction type payment waiver~~ Fineract-2092: new transaction type payment waiver Jun 13, 2024

somasorosdpc changed the title ~~Fineract-2092: new transaction type payment waiver~~ FINERACT-2092: new transaction type payment waiver Jun 13, 2024

somasorosdpc marked this pull request as ready for review June 14, 2024 07:07

adamsaghy changed the title ~~FINERACT-2092: new transaction type payment waiver~~ FINERACT-2092: New transaction type payment waiver Jun 14, 2024

somasorosdpc force-pushed the FINERACT-2092/new_transaction_type_payment-waiver branch from 79e3da2 to 5269d8e Compare June 14, 2024 09:12

adamsaghy reviewed Jun 14, 2024

View reviewed changes

adamsaghy requested changes Jun 14, 2024

View reviewed changes

somasorosdpc force-pushed the FINERACT-2092/new_transaction_type_payment-waiver branch 8 times, most recently from 8aaa1ec to 45b0c6d Compare June 19, 2024 08:51

somasorosdpc changed the title ~~FINERACT-2092: New transaction type payment waiver~~ FINERACT-2092: New transaction type interest payment waiver Jun 19, 2024

somasorosdpc force-pushed the FINERACT-2092/new_transaction_type_payment-waiver branch from 45b0c6d to 952b085 Compare June 19, 2024 08:53

adamsaghy reviewed Jun 19, 2024

View reviewed changes

somasorosdpc force-pushed the FINERACT-2092/new_transaction_type_payment-waiver branch 2 times, most recently from 7e3191a to 1316940 Compare June 20, 2024 14:14

FINERACT-2082: New Transaction Type - Interest Payment Waiver

580a8fb

somasorosdpc force-pushed the FINERACT-2092/new_transaction_type_payment-waiver branch from 1316940 to 580a8fb Compare June 20, 2024 14:26

adamsaghy reviewed Jun 21, 2024

View reviewed changes

adamsaghy approved these changes Jun 21, 2024

View reviewed changes

adamsaghy merged commit 9152408 into apache:develop Jun 21, 2024

somasorosdpc deleted the FINERACT-2092/new_transaction_type_payment-waiver branch July 17, 2024 15:43


		// Save changedTransactionDetail if exists

		if (changedTransactionDetail != null) {

FINERACT-2092: New transaction type interest payment waiver #3932

FINERACT-2092: New transaction type interest payment waiver #3932

Uh oh!

Conversation

somasorosdpc commented Jun 13, 2024

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adamsaghy Jun 14, 2024 •

edited

Loading

adamsaghy Jun 14, 2024 •

edited

Loading

adamsaghy Jun 14, 2024 •

edited

Loading