oleksii-novikov-onix commented on PR #6613: URL: https://github.com/apache/fineract/pull/6613#issuecomment-6215386917
> ## 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 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. #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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
