adamsaghy commented on PR #6235:
URL: https://github.com/apache/fineract/pull/6235#issuecomment-5954917182
Thanks for the fix. I checked each claim in the description against the code:
### 1. SI not created during loan disbursement — fix is in the list query,
not creation
`retrieveAll(StandingInstructionDTO)` is only called by `GET
/standinginstructions` (and the history resource). The code that creates the SI
at disbursement,
`LoanWritePlatformServiceJpaRepositoryImpl#createStandingInstruction`, is
unchanged and already saves it as savings → loan with `LOAN_REPAYMENT`. So the
SI was always being created; the real problem was that filtering the list by a
loan account found nothing. Please update the description to say that.
The list-filter fix itself is right, but there are gaps:
- **No `WHERE` keyword in the new fallback:** `" where "` is only added when
`transferType`, `clientId` or `clientName` is set. The `fromAccount` filter
isn't part of that check. So `GET
/standinginstructions?fromAccountId=X&fromAccountType=2` produces `... ON
toloanacc.product_id = tolp.id (toloanacc.id=? OR fromloanacc.id=?)` and
returns a 500. This existed before, but the new `transferType == null` fallback
was written for exactly this case. `StandingInstructionHistoryReadServiceImpl`
already handles it correctly.
- **History endpoint not fixed:**
`StandingInstructionHistoryReadServiceImpl` still filters on `fromloanacc.id=?`
only, so run history for a disbursement-created SI still can't be found by loan
account.
- **Other transfer types:** only `LOAN_REPAYMENT` uses `to_loan_account_id`.
Any other non-null `transferType` (e.g. `LOAN_DOWN_PAYMENT`) still uses
`from_loan_account_id` only. Consider using the `(toloanacc.id=? OR
fromloanacc.id=?)` form whenever `fromAccountType` is LOAN.
### 2. PostgreSQL boolean comparisons — looks good
`completed_derived`, `is_reversed` and `is_reversal` are boolean columns, so
`<> 1` fails on PostgreSQL. `retriveLoanDuesData` runs for every SI that pays
loan dues, including the ones created at disbursement. `IS FALSE` / `= false`
is correct and also works on MySQL/MariaDB.
### 3. Liquibase changeset `0245` — deletes the wrong rows (blocking)
The changeset deletes the codes **without** the trailing space and keeps
`'CREATE_STANDINGINSTRUCTION '` etc. But the application checks the no-space
codes:
- `CommandWrapper` builds the permission name as `actionName + "_" +
entityName` → `CREATE_STANDINGINSTRUCTION`.
- `Permission.hasCode` uses `equalsIgnoreCase`, which still compares the
trailing space, so the trailing-space codes never match.
- `0194_fix_missing_permission` (changesets 20–22) added the no-space codes
for exactly this reason.
What this does on upgrade:
- **If any role holds the no-space permission:** `m_role_permission` has a
foreign key with `onDelete="RESTRICT"`, so the `DELETE` fails. Because the
changeset has `failOnError="true"`, the tenant migration stops.
- **If no role holds it:** the delete succeeds, and from then on
non-superuser roles can't be granted create/update/delete SI rights. With
maker-checker turned on,
`ConfigurationDomainServiceJpa#isMakerCheckerEnabledForTask` calls
`findOneByCode("CREATE_STANDINGINSTRUCTION")`, gets `null` and throws
`PermissionNotFoundException` on every SI write, even for superusers.
The changeset should delete the **trailing-space** rows instead, after
moving any `m_role_permission` assignments over to the no-space row (merging,
not duplicating). The duplicates also exist on MySQL/MariaDB (both come from
`0002_initial_data` + `0194`), so the cleanup should run on all supported
databases, not just PostgreSQL. Note that the `0002` data also has
`READ_STANDINGINSTRUCTION ` with a trailing space.
### Tests
Please add tests covering:
- listing SIs filtered by a loan account (with and without `transferType`)
- the SI job running on PostgreSQL for a loan-dues SI
--
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]
