FINERACT-2455: working capital loan object parity refactor - #6303
FINERACT-2455: working capital loan object parity refactor#6303budaidev wants to merge 1 commit into
Conversation
a08e93e to
fe0fefc
Compare
galovics
left a comment
There was a problem hiding this comment.
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 useisClosed()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()?).WorkingCapitalLoanSummaryMappergets converted to the new helper, butWorkingCapitalLoanMapperstill does the oldfindFirst(nonNull)for the same field - since there's no@OrderBy, the two endpoints can now report differentactualDisbursementDatefor the same loan.
Recommendation: COMMENT
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!
Your assigned reviewer(s) will follow our guidelines for code reviews.