FINERACT-2743: Fix standing instruction not created at loan disbursem… - #6235
FINERACT-2743: Fix standing instruction not created at loan disbursem…#6235rymghosn wants to merge 1 commit into
Conversation
…ent and SI batch job failure - Correct account-column selection in StandingInstructionReadPlatformServiceImpl for loan accounts: a LOAN_REPAYMENT transfer stores the loan in to_loan_account_id (FROM=SAVINGS, TO=LOAN), not from_loan_account_id, so the standing instruction lookup was silently matching zero rows. - Replace integer boolean comparisons (<> 1) with proper boolean predicates in StandingInstructionReadPlatformServiceImpl and SavingsAccountTransactionRepository, which fail on PostgreSQL boolean columns and caused the standing instruction batch job to error out. - Add a Liquibase changeset to delete duplicate Standing Instruction permission rows that differ only by a trailing space in their code.
7ecdac8 to
733475a
Compare
|
@adamsaghy Could you please review the code? If everything looks good, kindly approve and merge it. Thanks! |
| 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" |
There was a problem hiding this comment.
Why to delete the non-spaced over the trailing space one?
Having trailing space in the permissions are clearly incorrect.
Should we consider removing the one with extra space instead?
There was a problem hiding this 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.
| } 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) |
There was a problem hiding this comment.
This look incorrect. In my understanding you are completely mixing the from and to accounts here...
There was a problem hiding this 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:
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.
| // 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=? "); |
There was a problem hiding this comment.
why to use the standingInstructionDTO.fromAccount() for toloanacc.id?
There was a problem hiding this 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.
| sqlBuilder.append(" toloanacc.id=? "); | ||
| paramObj.add(standingInstructionDTO.fromAccount()); | ||
| } else if (transferTypeValue == null) { | ||
| sqlBuilder.append(" (toloanacc.id=? OR fromloanacc.id=?) "); |
There was a problem hiding this comment.
why to check for to and from account as well the standingInstructionDTO.fromAccount()?
There was a problem hiding this 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.
adamsaghy
left a comment
adamsaghy
left a comment
There was a problem hiding this comment.
Please see my concerns
3 participants
Fixes two issues in the Standing Instruction (SI) feature:
Standing Instruction not created during loan disbursement
StandingInstructionReadPlatformServiceImplto correctly identify the loan account based on the transfer type.LOAN_REPAYMENTtransfers, the loan account is stored into_loan_account_idrather thanfrom_loan_account_id.Standing Instruction batch job failure on PostgreSQL
<> 1) with proper boolean predicates (IS FALSE/= false).Additionally, added a Liquibase changeset to remove duplicate
m_permissionentries forCREATE_STANDINGINSTRUCTION,UPDATE_STANDINGINSTRUCTION, andDELETE_STANDINGINSTRUCTIONthat differ only by a trailing space in their permission code. The changeset is PostgreSQL-only, idempotent, and safely executes only when duplicates exist.PR:(https://issues.apache.org/jira/browse/FINERACT-2743)
Testing
./gradlew :fineract-provider:compileJava— Passed./gradlew :fineract-core:compileTestJava :fineract-provider:compileTestJava— PassedStandingInstructionDataSerializationTestandExecuteOverdueAndCurrentStandingInstructionsTest) all pass.