SamaSVM commented on PR #6647:
URL: https://github.com/apache/fineract/pull/6647#issuecomment-6415710236
@galovics Thanks for the review.
**1. `concat('%', ?, '%')` → bound wildcards**
Agreed. This is simpler and matches the existing convention
(`ClientReadPlatformServiceImpl:177`, `GroupReadPlatformServiceImpl:211`,
`SearchReadServiceImpl:82`), where `"%" + param + "%"` is bound as a parameter:
```java
sql += " ( name like ? or gl_code like ? )";
final String searchPattern = "%" + searchParam + "%";
```
This also allowed me to remove the `sql.contains("concat(...)")` assertion,
which was unnecessarily tied to the exact SQL text.
**2. `ChartOfAccountsWorkbook` — good catch, and it's more than cosmetic**
This is a regression that the PR would have introduced.
Before the mapper fix, `codeId` for an untagged account was `0L` (due to the
`wasNull()` bug). As a result, `getTagId()` returned `CodeValueData(0L, null)`,
and the row was generated as `Name-1-null-0`. The `setLookupTable` logic
detected this through `accountNameAndTagAr.get(3).equals("0")`.
After `getTagId()` was correctly changed to return `null`, the row became
`Name-1-null-null`. The `"0"` check no longer matched, so the literal `"null"`
flowed into `LOOKUP_TAG_ID_COL` → VLOOKUP → `Long.parseLong("null")` in
`ChartOfAccountsImportHandler:134`, resulting in a `NumberFormatException`
during chart-of-accounts import.
I fixed this by writing an empty string instead:
```java
+ (tag == null ? "" : tag.getName()) + "-" + (tag == null ? "" : tag.getId())
```
I chose `""` rather than removing the tag suffix because `setLookupTable`
unconditionally accesses `get(2)` and `get(3)`, so a shorter string would
result in an `IndexOutOfBoundsException`.
With `""`, both branches fall through to their respective `else` clauses and
write `""` — the same cell value that the existing special-case branches
already produced. `ImportHandlerUtils.readAsString` then returns `null` for an
empty formula result, so `Long.parseLong` is never reached.
**3. `searchGLAccountsBySearchParamTest`**
Right — `name` and `glCode` had the same value, so the "found by glCode"
case never actually exercised the `gl_code like` branch.
I added a `createGLAccount(name, glCode)` overload and changed the two
accounts to use distinct `..._NAME` / `..._CODE` values. This ensures that the
second lookup can only match through `gl_code`.
I also moved the cleanup into `try/finally`.
**4. `GLAccountReadPlatformServiceImplTest`**
Agreed. I removed the hand-rolled `ResultSet` and the
`shouldMapMissingTagAsNullCodeId` test, since the integration test already
verifies `getTagId() == null` against a real database.
What's left is one short test that verifies the bound parameters without
depending on the SQL text:
```java
assertFalse(sql.contains("%?%"));
assertArrayEquals(new Object[] { "%cash%", "%cash%" },
paramsCaptor.getValue());
```
--
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]
