adamsaghy commented on PR #5895: URL: https://github.com/apache/fineract/pull/5895#issuecomment-5914860183
I reviewed [apache/fineract#5895](https://github.com/apache/fineract/pull/5895) against its own description. It doesn't deliver the headline case: a savings-to-loan instruction with a Dues amount type and "As per Dues" recurrence can't be created at all, and the build is red. ### What doesn't match the description **1. Dues + "As per Dues" (loan repayment) can't be created.** This is confirmed. - The factory sends this combination to `LoanRepaymentStandingInstruction`, which calls `validateAmountForFixedInstructionType()`. That check requires an amount that is present and positive. - I ran the real validator with the real helper and factory (a throwaway test, now deleted): - No amount: rejected with `amount.cannot.be.blank`. - `amount=10`: the validator accepts it. `StandingInstructionAssembler` then passes the amount into the entity constructor, where `validateDependencies` rejects it with `not.allowed.for.dues.instruction`. - So there's no request that works. The description's "Amount is now optional/prohibited when `instructionType` is `DUES`" only holds for Dues with periodic recurrence, which does pass without an amount. - The fix is probably to call `validateAmountForDuesInstructionType()` in that class. **2. The "robust unit test suite" doesn't pass: 38 of 75 fail.** - CI's `build-core` never got to the tests. It failed at compile on error-prone `[UnusedMethod]` for `isLoanAccount`, `isSavingsAccount` and `areEqualOfficesAndEqualAccounts`. Your uncommitted local edit removes exactly those three methods. - With that edit applied, every `WhenCreatingStandingInstruction` test fails with an NPE. The test mocks `StandingInstructionHelper` and `StandingInstructionValidatorFactory`, so `extractStandingInstruction` returns null. - Even with the NPE fixed, the mocked factory means the create tests never touch the new validator classes. `shouldPassWithTraditionalData` (Dues + As per Dues, no amount) would have caught issue 1 if they did. **3. Part of the update-path "decision matrix" isn't enforced.** I found this by reading the code, not by running it. - The Fixed + "As per Dues" check in `validateForUpdate` only runs when `recurrenceType` is in the request. - Take an existing Dues / As-per-Dues instruction and send only `instructionType=1` plus an amount. That gets past the validator. The entity's `validateDependencies` has no Fixed + As-per-Dues check either, so the instruction is saved as Fixed + As per Dues. **4. Changes the description doesn't mention:** - `gradle.properties` comments out `org.gradle.configuration-cache=true`. This looks like a local tweak and should come out. - `StandingInstructionData.getRecurrenceFrequencyOptions()` now drops frequency ids of 4 and above. - `retrieveTemplate` now offers only Fixed / Periodic options for account transfers. That's consistent with the new rules, just not described. ### What matches - All 12 error-code constants are added and used where the description says. - `validateForUpdate` now takes the existing entity and checks incoming values against it, including `must.be.before.existing.valid.till` and `cannot.be.before.last.run.date`. The update tests pass. - The entity's `update()` clears the amount for Dues, and clears frequency, interval and month-day for As-per-Dues recurrence. The description ties all of that clearing to switching to "Dues", but the recurrence fields are only cleared when recurrence is As per Dues. - `Objects.equals` replaces the NPE-prone `.equals` on `recurrenceOnDay`/`recurrenceOnMonth`. - `@Getter` is added, and the `latsRunDate` typo is fixed to `lastRunDate`. - Update now has `@Transactional` plus `saveAndFlush` with the duplicate-name handling. - Account-transfer create correctly rejects Dues, As per Dues, and transfers to the same account. Your working tree still has the uncommitted edit on `fix/FINERACT-2048`; I didn't commit or change anything. -- 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]
