Conversation
4a43bf8 to
56f3def
Compare
8bcd086 to
fef6c9e
Compare
fef6c9e to
bd2ac7f
Compare
galovics
left a comment
There was a problem hiding this comment.
Review
Tag transition logic looks fine to me, and existing loans with several active tags on one period clean themselves up on the next classification, so I don't see the need for a data migration. Couple of questions though.
fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/service/WorkingCapitalLoanDelinquencyReadPlatformServiceImpl.java (findDelinquencyStartDate / findPeriodDelinquencyStartDate):
I'm not convinced about walking the lifted tag chain to get the delinquency start date. The javadoc on
applyDelinquencyTagForRangeeven says the read side relies on the lift date matching the add date exactly. That's a hidden contract across two services, and the next person who touches the write side (reset, disable/undo, reprocess, etc.) will break it without noticing. The range schedule period already knows when it became delinquent (toDate+ delinquent days). Can't we get the start date from there, or store it on the period when the first tag is added? Thoughts?
fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/service/WorkingCapitalLoanDelinquencyRangeScheduleServiceImpl.java:140:
Bugfix? Switching from
transactionDateto the business date changes behaviour for backdated repayments, so it'd be good to mention it in the PR description. Also the comment at the end ofallocateRepaymentstill says "applyRepayment classifies after the balance cap with the real transaction date". Pls update it.
Recommendation: COMMENT
bd2ac7f to
02d1e1f
Compare
02d1e1f to
925690e
Compare
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.