FINERACT-2317: Add support for modifying loan approved amounts with validation and history tracking #4828

Cocoa-Puffs · 2025-07-01T11:14:36Z

Description

Added two new endpoints to handle loan approved amount modifications. Users can now lower the approved amount on loans, if the new approved amount is not lower than the actual and expected disbursements and capitalized income transactions. The history of these changes can also be examined by calling the new get endpoint with a loan id.

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 · 2025-07-02T14:58:08Z

.../apache/fineract/infrastructure/core/exception/PlatformApiDomainRuleValidationException.java

+import java.util.List;
+import org.apache.fineract.infrastructure.core.data.ApiParameterError;
+
+public class PlatformApiDomainRuleValidationException extends AbstractPlatformDomainRuleException {


Should GeneralPlatformDomainRuleException be used instead?

I wanted to create a new exception, that will be mapped to http status code 403 and behaves similarily to PlatformApiDataValidationException so that I can add multiple validation error messages to the exception. I can expand this exception instead.

Well... to me it looks like the love child of PlatformApiDataValidationException and GeneralPlatformDomainRuleException.

The difference is

GeneralPlatformDomainRuleException -> HTTP 403 and to be used if any domain rule violated like "You cannot set a future date", "You cannot reverse an already reversed transaction" etc.
PlatformApiDataValidationException -> HTTP 400 and to be used if the request contains unknown fields, or a mandatory field were not passed, or the value is incorrect, etc...

adamsaghy · 2025-07-02T14:58:30Z

...g/apache/fineract/infrastructure/core/exceptionmapper/PlatformDomainRuleExceptionMapper.java

        log.warn("Exception occurred", ErrorHandler.findMostSpecificException(exception));
-        final ApiGlobalErrorResponse notFoundErrorResponse = ApiGlobalErrorResponse.domainRuleViolation(
-                exception.getGlobalisationMessageCode(), exception.getDefaultUserMessage(), exception.getDefaultUserMessageArgs());
+        final ApiGlobalErrorResponse notFoundErrorResponse;


What was the problem here?

If it is a validation exception I want to include the error messages that were generated during validation. These validation messages are stored in the errors list in the exception, not in the defaultUserMessageArgs. I did it this way to keep the new PlatformApiDomainRuleValidationException consistent with PlatformApiDataValidationException. But I can change it.

I will replace the PlatformApiDomainRuleValidationException with the GeneralPlatformDomainRuleException mentioned in the other comment and remove this logic.

PlatformApiDomainRuleValidationException is mapped HTTP 403 exception for domain rule violations
PlatformApiDataValidationException is mapped to HTTP 400 exception for request data validation violations

adamsaghy · 2025-07-02T15:00:48Z

...src/main/java/org/apache/fineract/portfolio/loanaccount/serialization/LoanValidatorImpl.java

+
+@Component
+@RequiredArgsConstructor
+public final class LoanValidatorImpl implements LoanValidator {


This seems not a LoanValidator to me... We are validating whether approved amount can be changed....

I wanted to create a validator that in the future can be used to group validation logics relating to the loan entity itself. Like for example the LoanTransactionValidator for different loan transaction validation logics. I can rename it so it only relates to this use case.

adamsaghy · 2025-07-02T15:03:31Z

...src/main/java/org/apache/fineract/portfolio/loanaccount/serialization/LoanValidatorImpl.java

+            Loan loan = this.loanRepository.findById(loanId).orElseThrow(() -> new LoanNotFoundException(loanId));
+            BigDecimal approvedPrincipal = loan.getApprovedPrincipal();
+
+            if (MathUtil.isGreaterThan(newApprovedAmount, approvedPrincipal)) {


We should allow to increase approved amount if:

Applied amount + configured threshold equals or higher than "new approved" amount

new approved amount cannot be lower than already disbursed amount + total capitalized income

new approved amount cannot be lower than total "expected disbursement" + total capitalized income

adamsaghy · 2025-07-02T15:06:15Z

...src/main/java/org/apache/fineract/portfolio/loanaccount/serialization/LoanValidatorImpl.java

+            }
+
+            BigDecimal reservedPrincipal = BigDecimal.ZERO;
+            if (!loan.loanProduct().isDisallowExpectedDisbursements()) {


You can simplify this, by removing this condition and aggregate the principal of loan disbursement details (except the reversed=true entries). It will contains all already disbursed (actualDisbursementDate != null) and not yet disbursed but expected (actualDisbursementDate == null).

adamsaghy · 2025-07-02T15:07:32Z

...src/main/java/org/apache/fineract/portfolio/loanaccount/serialization/LoanValidatorImpl.java

+
+            reservedPrincipal = reservedPrincipal.add(loan.getSummary().getTotalPrincipal());
+
+            if (MathUtil.isLessThan(newApprovedAmount, reservedPrincipal)) {


I am not fond of "reserved principal" definition. Would "total already disbursed and expected disbursement amount is higher than new approved amount"?

adamsaghy

Kindly see my review!

adamsaghy

LGTM

fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/helper/ErrorMessageHelper.java

adamsaghy · 2025-07-16T11:43:48Z

fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/helper/ErrorMessageHelper.java

    }
+
+    public static String updateApprovedLoanExceedReservedPrincipalFailure() {
+        return "Failed data validation due to: less.than.reserved.principal.";


what is reserved principal?

adamsaghy · 2025-07-16T11:45:31Z

...rc/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanApprovedAmountHistory.java

+
+    @ManyToOne
+    @JoinColumn(name = "loan_id", nullable = false)
+    private Loan loan;


We dont need to fetch the Loan entity here... you can use Long type instead ;)

adamsaghy · 2025-07-16T11:46:28Z

...va/org/apache/fineract/portfolio/loanaccount/domain/LoanApprovedAmountHistoryRepository.java

+            )
+            FROM LoanApprovedAmountHistory laah
+            WHERE laah.loan.id = :loanId
+            ORDER BY laah.createdDate ASC


instead of hardcoding the order by you might wanna provide Pageable to this method and caller service can decide which order by to be used ;)

adamsaghy · 2025-07-16T11:48:01Z

...t-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/api/LoansApiResource.java

+        CommandWrapper commandRequest = builder.updateLoanApprovedAmount(resolvedLoanId).build();
+
+        final CommandProcessingResult result = this.commandsSourceWritePlatformService.logCommandSource(commandRequest);
+        return this.toApiJsonSerializer.serialize(result);


We can skip the serialization and return directly the CommandProcessingResult type.

...t-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/api/LoansApiResource.java

adamsaghy · 2025-07-16T11:48:54Z

...t-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/api/LoansApiResource.java

+        context.authenticatedUser().validateHasReadPermission(RESOURCE_NAME_FOR_PERMISSIONS);
+        Long resolvedLoanId = getResolvedLoanId(loanId, loanExternalId);
+        List<LoanApprovedAmountHistoryData> loanApprovedAmountHistory = loanApprovedAmountHistoryRepository.findAllByLoanId(resolvedLoanId);
+        return new LoanApprovedAmountHistoryResponse(loanApprovedAmountHistory);


We can return directly the List<LoanApprovedAmountHistoryData>, no need extra serialization.

adamsaghy · 2025-07-16T11:48:59Z

...t-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/api/LoansApiResource.java

+    @Consumes(MediaType.APPLICATION_JSON)
+    @Produces(MediaType.APPLICATION_JSON)
+    @Operation(summary = "Collects and returns the approved amount modification history for a given loan", description = "")
+    @ApiResponses({


Same as above

adamsaghy · 2025-07-16T11:50:32Z

.../java/org/apache/fineract/portfolio/loanaccount/serialization/LoanDisbursementValidator.java

                        || (totalDisbursed.add(capitalizedIncome).compareTo(loan.getApprovedPrincipal()) > 0)) {
-                    final String errorMsg = "Loan can't be disbursed,disburse amount is exceeding approved principal ";
-                    throw new LoanDisbursalException(errorMsg, "disburse.amount.must.be.less.than.approved.principal", totalDisbursed,
+                    final String errorMsg = "Loan can't be disbursed, disburse amount is exceeding proposed principal.";


Since we were checking the approved amount i think we should not change the exception message!

adamsaghy · 2025-07-16T11:51:36Z

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

                .build();
    }

+    @Override


LoanWritePlatformService getting bloated. Would you consider to move this into its own service?

adamsaghy · 2025-07-21T09:39:12Z

.../main/java/org/apache/fineract/portfolio/loanaccount/data/LoanApprovedAmountHistoryData.java

+    private BigDecimal oldApprovedAmount;
+    private OffsetDateTime dateOfChange;
+
+    public LoanApprovedAmountHistoryData(final Long loanId, final ExternalId externalLoanId, final BigDecimal newApprovedAmount,


@AllArgConstructor?

adamsaghy · 2025-07-21T09:39:33Z

...n/java/org/apache/fineract/portfolio/loanaccount/data/LoanApprovedAmountHistoryResponse.java

+
+    private List<LoanApprovedAmountHistoryData> approvedAmountHistory;
+
+    public LoanApprovedAmountHistoryResponse(final List<LoanApprovedAmountHistoryData> approvedAmountHistory) {


@AllArgConstructor?

adamsaghy · 2025-07-21T09:45:23Z

...er/src/main/resources/db/changelog/tenant/parts/0192_create_loan_approved_amount_history.xml

+    </changeSet>
+    <changeSet author="fineract" id="4" runInTransaction="false" context="postgresql">
+        <sql>
+            create index concurrently if not exists m_loan_approved_amount_history_idx_1 on m_loan_approved_amount_history(loan_id);


We dont need index if the column is already a foreign key!

adamsaghy

Please kindly see my review!

…alidation and history tracking

…proved amounts with validation and history tracking

adamsaghy

LGTM

Cocoa-Puffs force-pushed the FINERACT-2317-New-API-to-update-Approved-Loan-Amount branch from 7893d4e to 687fb7f Compare July 2, 2025 08:21

adamsaghy reviewed Jul 2, 2025

View reviewed changes

adamsaghy requested changes Jul 2, 2025

View reviewed changes

Cocoa-Puffs force-pushed the FINERACT-2317-New-API-to-update-Approved-Loan-Amount branch 2 times, most recently from ddb8a0f to bec1239 Compare July 3, 2025 12:25

adamsaghy force-pushed the FINERACT-2317-New-API-to-update-Approved-Loan-Amount branch from bec1239 to b328b63 Compare July 7, 2025 12:51

adamsaghy approved these changes Jul 7, 2025

View reviewed changes

budaidev force-pushed the FINERACT-2317-New-API-to-update-Approved-Loan-Amount branch 6 times, most recently from 799bc86 to 1034f4a Compare July 10, 2025 15:45

Cocoa-Puffs force-pushed the FINERACT-2317-New-API-to-update-Approved-Loan-Amount branch from 1034f4a to 54130c0 Compare July 15, 2025 13:48

adamsaghy reviewed Jul 16, 2025

View reviewed changes

fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/helper/ErrorMessageHelper.java Show resolved Hide resolved

adamsaghy reviewed Jul 16, 2025

View reviewed changes

...t-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/api/LoansApiResource.java Show resolved Hide resolved

adamsaghy reviewed Jul 16, 2025

View reviewed changes

Cocoa-Puffs force-pushed the FINERACT-2317-New-API-to-update-Approved-Loan-Amount branch from 54130c0 to e5f30be Compare July 16, 2025 16:11

Cocoa-Puffs force-pushed the FINERACT-2317-New-API-to-update-Approved-Loan-Amount branch from e5f30be to 699c865 Compare July 17, 2025 07:41

ruzeynalov force-pushed the FINERACT-2317-New-API-to-update-Approved-Loan-Amount branch from 0bba32d to 07c3b32 Compare July 17, 2025 15:00

Cocoa-Puffs marked this pull request as ready for review July 18, 2025 07:21

adamsaghy reviewed Jul 21, 2025

View reviewed changes

adamsaghy requested changes Jul 21, 2025

View reviewed changes

Cocoa-Puffs force-pushed the FINERACT-2317-New-API-to-update-Approved-Loan-Amount branch 2 times, most recently from 348752b to 1e5d9b2 Compare July 21, 2025 12:49

FINERACT-2317: Add support for modifying loan approved amounts with v…

a4973cd

…alidation and history tracking

Cocoa-Puffs force-pushed the FINERACT-2317-New-API-to-update-Approved-Loan-Amount branch from 54d4118 to b431f6e Compare July 21, 2025 14:16

FINERACT-2317: Added e2e tests covering support for modifying loan ap…

59a62ca

…proved amounts with validation and history tracking

adamsaghy force-pushed the FINERACT-2317-New-API-to-update-Approved-Loan-Amount branch from b431f6e to 59a62ca Compare July 21, 2025 14:39

adamsaghy approved these changes Jul 21, 2025

View reviewed changes

adamsaghy merged commit ecbefb0 into apache:develop Jul 21, 2025
39 checks passed

adamsaghy deleted the FINERACT-2317-New-API-to-update-Approved-Loan-Amount branch July 21, 2025 15:13


		reservedPrincipal = reservedPrincipal.add(loan.getSummary().getTotalPrincipal());

		if (MathUtil.isLessThan(newApprovedAmount, reservedPrincipal)) {


		private List<LoanApprovedAmountHistoryData> approvedAmountHistory;

		public LoanApprovedAmountHistoryResponse(final List<LoanApprovedAmountHistoryData> approvedAmountHistory) {

FINERACT-2317: Add support for modifying loan approved amounts with validation and history tracking #4828

FINERACT-2317: Add support for modifying loan approved amounts with validation and history tracking #4828

Uh oh!

Conversation

Cocoa-Puffs commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Cocoa-Puffs Jul 2, 2025 • 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!

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!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Cocoa-Puffs commented Jul 1, 2025 •

edited

Loading

Cocoa-Puffs Jul 2, 2025 •

edited

Loading