Skip to content

FINERACT-2455: working capital loan object parity refactor - #6303

Draft
budaidev wants to merge 1 commit into
apache:developfrom
openMF:FINERACT-2455/wc-loan-object-parity
Draft

FINERACT-2455: working capital loan object parity refactor#6303
budaidev wants to merge 1 commit into
apache:developfrom
openMF:FINERACT-2455/wc-loan-object-parity

Conversation

@budaidev

Copy link
Copy Markdown
Contributor

Description

Describe the changes made and why they were made. (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 our guidelines
  • 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 our 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
  • This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
  • If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

@budaidev
budaidev force-pushed the FINERACT-2455/wc-loan-object-parity branch from a08e93e to fe0fefc Compare August 21, 2026 08:03

@galovics galovics left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work on the test coverage for the status predicates - I walked every converted call site for isOpen/isApproved/isOverpaid/isClosedObligationsMet/isClosedWrittenOff/isSubmittedAndPendingApproval and they're all equivalent, including the null-status edge cases (several sites go from NPE-on-null to a clean false, which is a strict improvement).

getActualDisbursementDate() isn't behavior-preserving though, and I think that's worth settling before this comes out of draft. It merges two different pre-existing semantics: some call sites did filter(nonNull).findFirst() (arbitrary order, since disbursementDetails has no @OrderBy), but four sites in WorkingCapitalLoanDataValidator did getDisbursementDetails().getFirst().getActualDisbursementDate() - the first row's date whether or not it's null. The new helper does filter(nonNull).min(...) everywhere, which is a real behavior change at those four sites, and it's reachable: InternalWorkingCapitalLoanApiResource#activateLoan appends a new disbursement-detail row on activation rather than filling the existing expected-date row, so a loan can have row0 null and row1 populated. Before this PR validateDiscountTransaction would reject that with loan.not.disbursed; after this PR it doesn't. If that's an intentional bugfix, I'd rather see it as its own PR with a test pinning that specific case rather than hiding inside a "pure refactor."

Two smaller things:

  • isClosed()/isCancelled() have zero production call sites after this PR - and the two places that would naturally use isClosed() explicitly don't, each with a comment explaining why not to. That's a footgun with a warning sign next to it rather than a useful API. Either drop them until something needs them, or rename to something that doesn't require reading a comment to avoid the trap (isClosedOrCancelled()?).
  • WorkingCapitalLoanSummaryMapper gets converted to the new helper, but WorkingCapitalLoanMapper still does the old findFirst(nonNull) for the same field - since there's no @OrderBy, the two endpoints can now report different actualDisbursementDate for the same loan.

Recommendation: COMMENT

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.

2 participants