Skip to content

Conversation

@Harsh-Srivastav123
Copy link
Contributor

Description

This PR refactors several loan transaction methods to improve code clarity, maintainability, and performance. The changes include:

1. Refactoring doPostLoanTransactionChecks

  • Before: A single method handling both loan status checks and resetting the overpaid date.
  • After:
    • Extracted the status handling logic into checkAndHandleLoanStatus.
    • Introduced resetOverpaidDateIfNeeded to manage the overpaid date reset independently.

2. Optimizing getLastUserTransactionDate

  • Before: Iterative loop to determine the latest transaction date.
  • After:
    • Adopted a Stream-based approach to filter valid transactions and obtain the maximum date.
    • Added isValidTransaction helper method to clearly define valid transactions.

3. Simplifying isChargeOffOnDate

  • Before: Used a ternary operator to check for null and compare dates.
  • After:
    • Rewritten as a concise boolean expression for enhanced readability.

These improvements modularize the logic, making the code easier to read, maintain, and extend without altering existing functionality.

JIRA Issue: FINERACT-2080

(#1284).

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.

@a7med3del1973
Copy link
Contributor

I think you didn't see contribution guide...make sure you always include the proper Fineract jira ticket in the PR title and in the commit message as well.
Example:
https://issues.apache.org/jira/browse/FINERACT-2081: Minor bug-fixes and enhancements of 1.11.0

PR title and commit message must be the same.

.orElse(getDisbursementDate());
}

private boolean isValidTransaction(LoanTransaction transaction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont really like the name of this method... isValidTransaction is misleading... income posting and accrual related transactions are all "valid" transactions, however here they should not be considered... Maybe you want to use something that more descriptive about the filtering, like: "filter reverse and accrual related transactions" or something like that...

@Harsh-Srivastav123
Copy link
Contributor Author

Hi @adamsaghy @a7med3del1973

Thank you for the feedback and suggestions. I've noted the issues and will be making the required changes shortly. Please stay tuned for an updated version of the PR.

Thanks for your review!

@Harsh-Srivastav123
Copy link
Contributor Author

@adamsaghy please review the pr .

@a7med3del1973
Copy link
Contributor

@Harsh-Srivastav123
please squash your commits!

FINERACT-2080: Refactoring Loan entity
@Harsh-Srivastav123 Harsh-Srivastav123 force-pushed the Fineract-2080--refactor-loan-transactions branch from c34746f to 9c9254e Compare March 10, 2025 21:08
@Harsh-Srivastav123
Copy link
Contributor Author

@a7med3del1973 squashed commits is done .

@adamsaghy adamsaghy changed the title FINERACT-2080-Refactor Loan Transaction Checks and Related Methods FINERACT-2080: Refactor Loan Transaction Checks and Related Methods Mar 11, 2025
@adamsaghy adamsaghy merged commit c236ee0 into apache:develop Mar 14, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants