rymghosn commented on code in PR #6235: URL: https://github.com/apache/fineract/pull/6235#discussion_r3735865006
########## fineract-provider/src/main/resources/db/changelog/tenant/parts/0245_delete_trailing_space_standinginstruction_permissions.xml: ########## @@ -0,0 +1,75 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + +--> +<!-- + Liquibase changeSet to safely delete duplicate m_permission rows + for Standing Instruction permissions WITHOUT trailing spaces, + keeping the legacy trailing-space versions. + + Deletes ONLY: + - 'CREATE_STANDINGINSTRUCTION' + - 'UPDATE_STANDINGINSTRUCTION' + - 'DELETE_STANDINGINSTRUCTION' + + Safety rules: + - Delete ONLY if a trailing-space version already exists. + - Do NOT modify or insert any permissions. + - Do NOT touch m_role_permission. + - Idempotent and safe to re-run. + - PostgreSQL only. +--> +<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.1.xsd";> + + <changeSet id="0245_delete_non_spaced_standinginstruction_permissions" Review Comment: Great question — the trailing space is definitely wrong, but it's actually the *original* value, not the new one. `0002_initial_data.xml` (the base seed data every tenant has run since Fineract's very first release) inserts `'CREATE_STANDINGINSTRUCTION '`, `'UPDATE_STANDINGINSTRUCTION '` and `'DELETE_STANDINGINSTRUCTION '` **with** the trailing space (ids 450-452). The non-spaced duplicate was introduced much later, by accident, in `0194_fix_missing_permission.xml` (changeSets 20-22): that changeset's precondition does an exact-match check (`code = 'CREATE_STANDINGINSTRUCTION'`), which — because the existing row already has the trailing space — never matches, so it always reports "0 rows found" and inserts a second, non-spaced row on every tenant that has run through the changelog. Since `m_role_permission` references a permission by its numeric id, not by its code, any tenant that's been running since before changeset 0194 could already have granted Create/Update/Delete Standing Instruction to a custom role against the *original* (spaced) id. This changeset explicitly promises not to touch `m_role_permission`, so the only row we can safely delete without needing a role-permission migration is the newer accidental duplicate — which is what it does. It's also not just cosmetic: `PermissionRepository.findOneByCode()` already TRIM/LOWER-cases both sides of the comparison specifically because of this bug (see the comment on that method) — so with both rows present, any code path resolving this permission "by code" today throws a `NonUniqueResultException`, not just an ugly space. Properly fixing the underlying ~decade-old trailing space (e.g. `UPDATE m_permission SET code = TRIM(code)` on the surviving row) is a good idea, but it's a separate, larger change — it'd need a sweep to confirm nothing else compares against the exact `'... '` string, and probably deserves its own ticket rather than riding along with this SI disbursement fix. Happy to file that as a follow-up if useful. ########## fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/StandingInstructionReadPlatformServiceImpl.java: ########## @@ -299,8 +299,21 @@ public Page<StandingInstructionData> retrieveAll(final StandingInstructionDTO st sqlBuilder.append(" fromsavacc.id=? "); paramObj.add(standingInstructionDTO.fromAccount()); } else if (PortfolioAccountType.LOAN.equals(accountType)) { - sqlBuilder.append(" fromloanacc.id=? "); - paramObj.add(standingInstructionDTO.fromAccount()); + // For LOAN_REPAYMENT transfers, loan is stored in to_loan_account_id (FROM=SAVINGS, TO=LOAN) Review Comment: Totally fair to double check — the naming here is genuinely confusing, but it's not a mix-up. `standingInstructionDTO.fromAccount()` / `fromAccountType()` at this point in the method are the API's generic search filter ("give me standing instructions involving this account"), not literally "the source side of the money movement." Which physical column a loan account actually lives in depends on the transfer type, not on how the caller happened to name the query parameter. For the standing instructions this PR is about — the ones auto-created at loan disbursement — `LoanWritePlatformServiceJpaRepositoryImpl.createStandingInstruction()` builds the transfer via: ```java AccountTransferDetails.savingsToLoanTransfer(fromOffice, fromClient, linkedSavingsAccount, toOffice, toClient, loan, transferType); ``` That factory always stores the savings account as `fromSavingsAccount` and the loan as `toLoanAccount` (`from_loan_account_id` is left `null`). So for `LOAN_REPAYMENT` standing instructions, the loan is *always* on the `to_loan_account_id` side in the database — never `from_loan_account_id`. The old code compared the caller's loan-id filter against `fromloanacc.id` (i.e. `from_loan_account_id`), which is always null for these rows, so it could never find the auto-created standing instruction for a given loan. That's the actual "not found" symptom from the bug report. The fix just routes the comparison to whichever column the loan is genuinely stored in, based on the same `transferType` the write path used to create it. ########## fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/StandingInstructionReadPlatformServiceImpl.java: ########## @@ -299,8 +299,21 @@ public Page<StandingInstructionData> retrieveAll(final StandingInstructionDTO st sqlBuilder.append(" fromsavacc.id=? "); paramObj.add(standingInstructionDTO.fromAccount()); } else if (PortfolioAccountType.LOAN.equals(accountType)) { - sqlBuilder.append(" fromloanacc.id=? "); - paramObj.add(standingInstructionDTO.fromAccount()); + // For LOAN_REPAYMENT transfers, loan is stored in to_loan_account_id (FROM=SAVINGS, TO=LOAN) + // For other transfer types, loan is stored in from_loan_account_id + // Defensive fallback: if transferType is null, check both columns to handle UI inconsistencies + Integer transferTypeValue = standingInstructionDTO.transferType(); + if (transferTypeValue != null && transferTypeValue.equals(AccountTransferType.LOAN_REPAYMENT.getValue())) { + sqlBuilder.append(" toloanacc.id=? "); Review Comment: Because `fromAccount()` here is the loan id the *caller* is searching for (e.g. "show me the standing instruction for loan #42"), not literally the from-side of the money movement. As shown in `AccountTransferDetails.savingsToLoanTransfer(...)` (used by `LoanWritePlatformServiceJpaRepositoryImpl.createStandingInstruction()`), for `LOAN_REPAYMENT` transfers the loan is persisted as `toLoanAccount` / `to_loan_account_id` — `from_loan_account_id` is left null for these rows. So to find the standing instruction for loan #42 we have to match `toloanacc.id = 42`, even though the caller supplies that same loan id via the `fromAccount` parameter. I agree the parameter name is misleading — it really means "the account of interest" rather than "the from side" — but renaming it would touch the public search API's query params, which is a bigger, more invasive change than this bugfix needs. ########## fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/StandingInstructionReadPlatformServiceImpl.java: ########## @@ -299,8 +299,21 @@ public Page<StandingInstructionData> retrieveAll(final StandingInstructionDTO st sqlBuilder.append(" fromsavacc.id=? "); paramObj.add(standingInstructionDTO.fromAccount()); } else if (PortfolioAccountType.LOAN.equals(accountType)) { - sqlBuilder.append(" fromloanacc.id=? "); - paramObj.add(standingInstructionDTO.fromAccount()); + // For LOAN_REPAYMENT transfers, loan is stored in to_loan_account_id (FROM=SAVINGS, TO=LOAN) + // For other transfer types, loan is stored in from_loan_account_id + // Defensive fallback: if transferType is null, check both columns to handle UI inconsistencies + Integer transferTypeValue = standingInstructionDTO.transferType(); + if (transferTypeValue != null && transferTypeValue.equals(AccountTransferType.LOAN_REPAYMENT.getValue())) { + sqlBuilder.append(" toloanacc.id=? "); + paramObj.add(standingInstructionDTO.fromAccount()); + } else if (transferTypeValue == null) { + sqlBuilder.append(" (toloanacc.id=? OR fromloanacc.id=?) "); Review Comment: That branch only runs when `transferType` is `null`, i.e. the caller didn't filter by transfer type at all — they just asked for standing instructions touching a given loan account, regardless of type. Since which column (`to_loan_account_id` vs `from_loan_account_id`) holds the loan id depends on the transfer type (see the LOAN_REPAYMENT case above, where it's always `to_loan_account_id`), and we don't know the transfer type in this branch, we can't know in advance which single column to check — so we check both with `OR`, using the same caller-supplied loan id on each side. If a transfer type *is* given, we're back in one of the other two branches where we already know which single column applies. -- 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]
