Repository navigation
FINERACT-2455: Add working capital loan version of trial balance summary report and support for year end retained earning job - #6610
Conversation
fa29bcc to
7097997
Compare
galovics
left a comment
There was a problem hiding this comment.
Review
Hi @Cocoa-Puffs, thanks for the PR. The report SQL looks right to me: the WC report is the current 0237 report with the expected substitutions (WC tables, entity type 6, 'self' as asset owner), and the loan report change in 0259 is just AND e.product_type = 1, which is the correct fix because the two product id spaces are separate. A few things need fixing before this can merge.
fineract-provider/src/main/resources/db/changelog/tenant/changelog-tenant.xml:
0258collides with0258_add_read_entitymapping_permission.xmlon develop (the PR isCONFLICTING). Please rebase and shift the 4 new parts to 0259-0262.
WorkingCapitalLoanReporting.feature (lines 9, 41, 75) and WorkingCapitalLoanYearEndRetainedEarning.feature (line 12):
The new scenarios only have
# TODO: add TestRail IDplaceholders. All the otherWorkingCapital*.featurefiles carry@TestRailIdtags, pls add real IDs.
JournalEntryAggregationJobReader.java:102,166 / parts/0260_trial_balance_summary_report_for_working_capital_loans.xml:86,290:
Before this PR the aggregation job already picked up entity_type 6 entries but with no product match, so they were stored with productId NULL/0 and no originator ids. The new baseline query does
JOIN m_wc_loan_product lp ON lp.id = ags.product_id, which drops those rows, and the report only reads raw entries after the latest aggregation date, so they're never picked up again. A tenant that ran aggregation with WC loans before upgrading would see understated opening balances. Either add a migration that resets/re-aggregates the entity_type 6 rows, or state explicitly that there's no WC aggregation history in production.
RetainedEarningJobWriter:
It decides whether to write by
findByYearEndDate(...)for the whole year, not per product type. If the regular-loan close for a year ran before the upgrade, that year's WC close is silently a dry run. Probably acceptable, but let's document it.
Smaller ones:
- About 8 private helpers are copied verbatim from
FeignTrialBalanceSummaryReportTestintoFeignWorkingCapitalTrialBalanceSummaryReportTest(runReport,money,findColumnIndex,parseBigDecimal, the retained-earning config snapshot/restore, ...). Move them to a shared helper. - There are now two copies of a ~210-line report SQL that need parallel maintenance. Worth a sentence in the description.
- #6613 also changes
ReportsTest.listReports(85 to 87, this one 85 to 86), so whichever lands second must set it to 88. - The PR description is the untouched template.
Recommendation: REQUEST_CHANGES
61f397f to
eca6d7f
Compare
|
I've updated the liquibase numbering during the rebase. I do not have access to testrail, once QA picks up the ticket those scenarios will get testrail ids. As for the JournalEntryAggregationJobReader, RetainedEarningJobWriter concerns: That was a judgement call on my part. As WC loans were heavily WIP throughout the year and we are now reaching a point where they are close to being feature complete I assumed WC loans are not yet used in live environments and a migration is not necessary for that reason, plus the year end retained earning job is designed to run once a year at the end of it. So I do not think a migration is necessary for this. But I'll happily implement it if you think it is needed About 8 private helpers are copied verbatim from FeignTrialBalanceSummaryReportTest into FeignWorkingCapitalTrialBalanceSummaryReportTest (runReport, money, findColumnIndex, parseBigDecimal, the retained-earning config snapshot/restore, ...). Move them to a shared helper. agreed, moved them to a shared helper |
galovics
left a comment
There was a problem hiding this comment.
Review
Thanks for the updates @Cocoa-Puffs. The rebase is fine: 0259 was taken on develop in the meantime, so 0260-0263 is the right numbering. The shared FeignTrialBalanceReportHelper looks good too, and CI is green.
On the aggregation / retained earning points, I'm fine with your reasoning. WC loans haven't been usable in production yet, so there's no pre-existing entity_type 6 aggregation history to fix up, and no earlier WC year-end close that could turn into a silent dry run. No migration needed. The one thing I'd ask: pls fill in the PR description (it's still the untouched template). Put that assumption in there along with a sentence about the WC report being a parallel copy of 0237 that has to be maintained alongside it. That way it's documented if someone hits this later.
TestRail IDs: understood that you don't have access. The 4 scenarios still only have # TODO: add TestRail ID, and no other feature file on develop has that, so let's get the IDs from QA before this merges (or at least track it).
Just a heads-up, nothing to do here: #6613 is still open and also bumps ReportsTest.listReports. Whichever of the two merges second needs to set the count to 88.
Thanks!
Recommendation: COMMENT
|
@Cocoa-Puffs Please rebase onto the latest |
eca6d7f to
39db02c
Compare
…ary report and support for year end retained earning job
39db02c to
edb8bd6
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.