FINERACT-2455: Transaction Summary reports for Working Capital loans

[oleksii-novikov-onix]

Description

WC loans keep their transactions in separate tables, so the regular Transaction Summary reports never show them. This PR adds the WC versions of these reports:

1. Transaction Summary Report with Asset Owner for Working Capital Loans - separate SQL for PostgreSQL and MySQL.
2. Transaction Summary Report for Working Capital Loans - the plain version, one SQL for both.

Same parameters, columns, grouping, sign and reversal rules as the regular reports, plus r_enum_value labels for types 44-47. Only the WC transaction types in scope are reported. The office comes from the loan’s client, since WC transactions have no office_id. WC loans cannot be transferred, so the owner columns are always NULL.

Tests: WorkingCapitalLoanTransactionSummaryReportTest (MySQL, MariaDB, PostgreSQL) and WorkingCapitalTransactionSummaryReport.feature (PostgreSQL).

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.

[galovics]

Review

Hi @oleksii-novikov-onix, thanks for the PR. The SQL looks right to me: the WC joins, the reversal handling (original rows on submitted_on_date, opposite-sign rows on reversed_on_date), the r_enum_value ids 44-47 and the MySQL/PostgreSQL split all line up with the existing reports. CI is green and 0256 fills the gap before 0257. A couple of things need fixing though.

fineract-e2e-tests-runner/src/test/resources/features/WorkingCapitalTransactionSummaryReport.feature (lines 6, 254, 602, 816):

None of the 4 scenarios has a @TestRailId. Every other WorkingCapital*.feature and Reporting.feature tags each scenario. Pls add real IDs.

parts/0256_transaction_summary_reports_for_working_capital_loans.xml (the CTEs, e.g. lines 88, 141, 726, 779):

The type filter is a blacklist (NOT IN (10, 26)). Any other type then lands in a row with a NULL Transaction_Amount, because it's in neither sign list ((2,6,8,9,22,23,26,27,46,47) / (1,10,20,44,45)). The regular report deliberately excludes (10, 26, 29, 30, 32, 34, ...). ACCRUAL_ADJUSTMENT (34) is already listed in WorkingCapitalLoanTransactionFinder and WorkingCapitalLoanTransactionTypeExternalEvents. I can't find code that creates it for WC today so it's not a live bug, but I'd rather whitelist the types this report supports than find out when the next WC type is added. Thoughts?

0256...xml:185-186:

The "with Asset Owner" report always returns NULL Asset_owner_id/From_asset_owner_id. If that's only to keep the column shape of the regular report for downstream consumers, a one-line SQL comment would help. Otherwise the report name is misleading.

Smaller ones:

• ReportingStepDef: verifyReportDataHeaders, verifyColumnNullability (for-loop to IntStream.forEach) and sumColumnForGlAccount were rewritten, but only the new WC steps and verifyReportDataWithEmptyValues are needed. Harmless, but it widens the diff of shared step code.
• WorkingCapitalLoanTransactionSummaryReportTest (842 lines, 8 tests) overlaps heavily with the new 996-line e2e feature (types, reversals, waiver, charge-off reason, regular-vs-WC). Do we need both?
• The WC reports filter on the client's current office (cl.office_id) since m_wc_loan_transaction has no office column, so a client transfer would move historical rows. Probably fine for now, just worth knowing.
• Heads up: #6610 also changes ReportsTest.listReports (85 to 86, this one 85 to 87), so whichever lands second must set it to 88. Both touch ReportingStepDef and the changelog-tenant.xml tail too.
• PR description is the empty template.

Recommendation: REQUEST_CHANGES

[oleksii-novikov-onix]

Review

Hi @oleksii-novikov-onix, thanks for the PR. The SQL looks right to me: the WC joins, the reversal handling (original rows on submitted_on_date, opposite-sign rows on reversed_on_date), the r_enum_value ids 44-47 and the MySQL/PostgreSQL split all line up with the existing reports. CI is green and 0256 fills the gap before 0257. A couple of things need fixing though.

fineract-e2e-tests-runner/src/test/resources/features/WorkingCapitalTransactionSummaryReport.feature (lines 6, 254, 602, 816):

None of the 4 scenarios has a @TestRailId. Every other WorkingCapital*.feature and Reporting.feature tags each scenario. Pls add real IDs.

parts/0256_transaction_summary_reports_for_working_capital_loans.xml (the CTEs, e.g. lines 88, 141, 726, 779):

The type filter is a blacklist (NOT IN (10, 26)). Any other type then lands in a row with a NULL Transaction_Amount, because it's in neither sign list ((2,6,8,9,22,23,26,27,46,47) / (1,10,20,44,45)). The regular report deliberately excludes (10, 26, 29, 30, 32, 34, ...). ACCRUAL_ADJUSTMENT (34) is already listed in WorkingCapitalLoanTransactionFinder and WorkingCapitalLoanTransactionTypeExternalEvents. I can't find code that creates it for WC today so it's not a live bug, but I'd rather whitelist the types this report supports than find out when the next WC type is added. Thoughts?

0256...xml:185-186:

The "with Asset Owner" report always returns NULL Asset_owner_id/From_asset_owner_id. If that's only to keep the column shape of the regular report for downstream consumers, a one-line SQL comment would help. Otherwise the report name is misleading.

Smaller ones:

• ReportingStepDef: verifyReportDataHeaders, verifyColumnNullability (for-loop to IntStream.forEach) and sumColumnForGlAccount were rewritten, but only the new WC steps and verifyReportDataWithEmptyValues are needed. Harmless, but it widens the diff of shared step code.
• WorkingCapitalLoanTransactionSummaryReportTest (842 lines, 8 tests) overlaps heavily with the new 996-line e2e feature (types, reversals, waiver, charge-off reason, regular-vs-WC). Do we need both?
• The WC reports filter on the client's current office (cl.office_id) since m_wc_loan_transaction has no office column, so a client transfer would move historical rows. Probably fine for now, just worth knowing.
• Heads up: FINERACT-2455: Add working capital loan version of trial balance summary report and support for year end retained earning job #6610 also changes ReportsTest.listReports (85 to 86, this one 85 to 87), so whichever lands second must set it to 88. Both touch ReportingStepDef and the changelog-tenant.xml tail too.
• PR description is the empty template.

Recommendation: REQUEST_CHANGES

Thanks for the review, @galovics.

1. TestRail IDs: added.
2. Type filter: agreed, switched to a whitelist in all three versions (PostgreSQL, MySQL, plain): 1, 2, 6, 8, 9, 20, 22, 23, 27, 44-47, plus the accrual (10) and charge adjustment (26) branches - the same set as the sign lists. Any other type, e.g. 34, is now left out.
3. Asset owner columns: yes, only to keep the regular report’s output - the ticket asks for both columns, always NULL, as the regular report returns for loans without an owner. Added a one-line SQL comment. The name keeps “with Asset Owner” to pair with the regular report and not clash with the plain WC one.
4. ReportingStepDef: the three methods and the empty-value check in verifyReportDataRowsWithEmptyValues are back to develop. Only the WC steps, the verifyReportDataWithEmptyValues overload and the two-originator placeholder for the new scenario remain.
5. Integration test vs e2e: I’d keep both - the ticket asks for both, and they cover different things. The ITs run on MySQL, MariaDB and PostgreSQL and are the only coverage of the MySQL SQL; the e2e runs on PostgreSQL and is the only coverage of the plain WC report. The regular report is tested the same way.
6. Office: yes, by design - WC transactions have no office_id, so the ticket filters through the client. Noted in the description.
7. FINERACT-2455: Add working capital loan version of trial balance summary report and support for year end retained earning job #6610: noted. Liquibase parts have to stay sequential, so ours is now 0260, the next free number after develop’s 0259. Whichever PR lands second bumps its part numbers, sets the report count to 88 and merges the ReportingStepDef constants.
8. Description: filled in.

[galovics]

Review

Thanks @oleksii-novikov-onix, all addressed. The whitelist is in all three reports and matches the sign lists, the asset owner comment is there, the ReportingStepDef changes are back to only what's needed, and the TestRail IDs are added. Fair point on keeping both the IT and the e2e, since the IT is the only MySQL coverage.

One heads up: #6610 now uses 0260-0263 as well, so whichever lands second has to renumber its parts and set the report count to 88, like you said. Other than that, nice work.

Recommendation: APPROVE