Skip to content

FINERACT-2455: Add working capital loan version of trial balance summary report and support for year end retained earning job - #6610

Open
Cocoa-Puffs wants to merge 1 commit into
apache:developfrom
openMF:FINERACT-2455-trial-balance-wc-summary-report
Open

Cocoa-Puffs wants to merge 1 commit into
apache:developfrom
openMF:FINERACT-2455-trial-balance-wc-summary-report

Conversation

@Cocoa-Puffs

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.
  • I followed the AI Policy.

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

@Cocoa-Puffs
Cocoa-Puffs force-pushed the FINERACT-2455-trial-balance-wc-summary-report branch 3 times, most recently from fa29bcc to 7097997 Compare October 15, 2026 09:43

@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.

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:

0258 collides with 0258_add_read_entitymapping_permission.xml on develop (the PR is CONFLICTING). 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 ID placeholders. All the other WorkingCapital*.feature files carry @TestRailId tags, 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 FeignTrialBalanceSummaryReportTest into FeignWorkingCapitalTrialBalanceSummaryReportTest (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

@Cocoa-Puffs
Cocoa-Puffs force-pushed the FINERACT-2455-trial-balance-wc-summary-report branch 2 times, most recently from 61f397f to eca6d7f Compare October 16, 2026 09:00
@Cocoa-Puffs

Copy link
Copy Markdown
Contributor Author

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 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.

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

@adamsaghy

Copy link
Copy Markdown
Contributor

@Cocoa-Puffs Please rebase onto the latest develop and resolve the conflicts.

@Cocoa-Puffs
Cocoa-Puffs force-pushed the FINERACT-2455-trial-balance-wc-summary-report branch from eca6d7f to 39db02c Compare October 19, 2026 07:09
…ary report and support for year end retained earning job
@Cocoa-Puffs
Cocoa-Puffs force-pushed the FINERACT-2455-trial-balance-wc-summary-report branch from 39db02c to edb8bd6 Compare October 19, 2026 09:13
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