adamsaghy commented on PR #6116: URL: https://github.com/apache/fineract/pull/6116#issuecomment-5915022168
@Samer-Melhem-FOO I reviewed [apache/fineract#6116](https://github.com/apache/fineract/pull/6116) against what its description claims. CI is green. Everything below comes from reading the code at `49f2fc7`; I didn't run it. The main fix doesn't hold on PostgreSQL, and two other claims don't match the code. ## Where the implementation falls short **1. On PostgreSQL the fix doesn't work for datatables registered with a subtype.** - The PR looks up the Create checks with the subtype `"Person"` / `"Entity"`. It does this in three places: `BulkImportWorkbookPopulatorServiceImpl#populateClientWorkbook`, and the `validateRequiredDatatables` and `readDatatablesFromRowWithAllConfigured` calls in both import handlers. - That lookup, `EntityDatatableChecksRepository.findByEntityAndStatusAndSubtype`, is a plain `rdt.subtype = :subtype` comparison against the value stored when the datatable was registered. The stored value is saved exactly as sent, and the convention is `"PERSON"` / `"ENTITY"`: the integration tests use it, and the server-side `runTheCheck` upper-cases the label before querying. - On PostgreSQL that comparison is case-sensitive, so the PR's lookup finds nothing. The template gets no datatable columns and the handler sends no `datatables`. Then `ClientWritePlatformServiceJpaRepositoryImpl` calls `runTheCheck("PERSON")`, finds the check, and throws `DatatableEntryRequiredException`. That is the same failure the PR is meant to fix. - MySQL and MariaDB compare case-insensitively by default, which may be why the manual test passed. The fix is to pass `LegalForm.X.getLabel().toUpperCase()`, the same value the server uses. **2. "Including ones with no existing check, so they can be optionally populated" isn't implemented.** `fetchRequiredDatatables` only collects datatables that have a Create-status check, and that list is the only thing handed to the populators. `EntityDatatableChecksReadService` is injected into the service but never used. Datatables on the Client entity without a check never appear in the template. **3. Multi-row datatables being "always optional" conflicts with the server's check.** - A multi-row datatable only appears in the template because it has a Create check, and `runTheCheck` requires at least one row in it. - If the user leaves it blank and it has any non-nullable column, `readDatatablesFromRowWithAllConfigured` leaves it out of the payload, so the row fails with `DatatableEntryRequiredException`. The template shows no `*` on those columns, so the user gets no warning. - If all its columns are nullable, the handler sends a row of empty strings. This goes for single-row datatables too: every imported client gets a blank datatable row, which satisfies the check without the user entering anything. The description doesn't mention this. **4. Testing.** `DatatableDataTest` only checks the new constructor argument and getters. No test covers the template columns, parsing values from the sheet, or importing with a datatable check in place. The only integration test change adapts `ClientEntityImportHandlerTest` to the lookups moving to a hidden sheet. A PostgreSQL integration test with a `"PERSON"` datatable check would have caught #1. ## Claims that match the code - Datatable columns are named `table.column`; non-nullable columns of single-row datatables get `*`, multi-row ones never do. - Dropdown, date, datetime and boolean columns get their Excel validations. Datetime values arrive as dates at midnight, and the server's parser accepts that. - `validateRequiredDatatables` skips multi-row datatables, and the handlers add the parsed `datatables` to the create-client payload. - `DatatableData.isMultiRow()` is set in `DatatableReadServiceImpl` and `ReadSurveyServiceImpl`. ## Changes the description doesn't mention - **Generic datatable API:** `SearchUtil.parseColumnValue` now turns any `"Label (123)"` value in an integer or code-lookup column into `123`. This applies to every datatable write, not just bulk import. - **Template layout:** the lookup lists moved to a new hidden sheet, `_CLIENT_LOOKUPS`, and dropdown values changed from `Name-12` to `Name (12)`. Templates downloaded before this change will quietly lose client type, gender, classification, address type, state and country (the handlers log a warning and send null). Constitution is handled differently: a bad value now fails the row. - **Stale comment:** the TODO says the lookup sheet is "temporarily set to VISIBLE", but the code sets it to `VERY_HIDDEN`. #1 is the blocker, since the bug is still there on PostgreSQL. I can write up the review comment, or put the subtype fix on `port/CBS-1-bulk-client-upload-custom-fields` if you want. -- 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]
