meetme-venkat opened a new pull request, #6806: URL: https://github.com/apache/fineract/pull/6806
## Description JIRA ticket requested on [email protected]; I will add the key to the commit message once assigned. ### Problem (at 8e80aa6) `ExecuteStandingInstructionsTasklet` ran every due instruction inside the single Spring Batch step transaction and built the history INSERT by string concatenation. As a result: - When one instruction failed, the job threw at the end and the step transaction rolled back. That undid the transfers, history rows and `last_run_date` updates of the instructions that had succeeded in the same run, and also the `failed` history row itself. - An error message containing an apostrophe (e.g. `customer's account`) produced invalid SQL (`BadSqlGrammarException`). That aborted the run before later instructions were processed. - `amount` was written via `BigDecimal.doubleValue()`, so values such as `9000000000000.000001` lost precision. ### What already existed vs. what this PR adds | | Existed at 8e80aa6 | Added here | |---|---|---| | Partial transfers (`allow_partial_transfer`) | Yes. Liquibase `0273_add_allow_partial_transfer_to_standing_instruction.xml` (default `false`), API field `allowPartialTransfer` | Nothing. **No Liquibase changeset is needed.** | | `partial` history status | Yes (`StandingInstructionHistoryStatus.PARTIAL`) | Nothing | | `last_run_date` column | Yes (`0001_initial_schema.xml`) | Exposed read-only as `lastRunDate` on `GET /standinginstructions/{id}` (`StandingInstructionData`, read mapper, Swagger, `apiLive.htm` example) so the integration test can assert it | | Per-instruction isolation | No | Each instruction runs in its own `REQUIRES_NEW` transaction (existing `requiresNewTransactionTemplate` bean). A success or partial transfer commits together with its history row and `last_run_date`. A failed transfer is rolled back, and its `failed` history row is written in a separate transaction. | | Job reported as failed | Yes (`JobExecutionException` at the end) | Unchanged: still thrown when any instruction failed | | History INSERT | String-concatenated, `double` amount; an error text over 500 characters made the INSERT fail | Parameterized, `BigDecimal` amount. The error text is truncated to the `error_log` column length (`VARCHAR(500)`, via `StringUtils.abbreviate`) before the insert. | | History amount semantics | — | success/partial = amount posted; failed = amount attempted | ### Tests 1. **Mixed outcomes in one scheduled run (PostgreSQL integration test)**: `integration-tests/.../savings/StandingInstructionMixedOutcomeTest`. - Sets up three instructions A, B and C, each with its own savings pair. The product has no interest, overdraft, minimum balance or withholding tax. - Priorities are LOW/MEDIUM/HIGH; the due query orders by `priority DESC`, so they run A, B, C. - Monthly on the 15th, `valid_from` 2026-02-01, job business date 2026-03-15, set via the business date API (`runAt`). - Records the initial history counts, `last_run_date` and transaction counts, then triggers the job through the scheduler and waits for that run. - **Job outcome is asserted on the run's `job_run_history` entry** (written by `SchedulerJobListener` and read via `GET /jobs/{jobId}/runhistory`): `status == "failed"`. To return that entry without failing on a non-success status, `FeignSchedulerHelper` gained `executeAndAwaitJobRun`; `executeAndAwaitJob` keeps its behaviour. - Then asserts: A `success 40.00`, balances 60.00/40.00, one transfer, `last_run_date` = job date. B `failed 40.00`, `error_log` contains `InsufficientAccountBalance`, no debit or credit, balances 10.00/0.00, `last_run_date` unchanged (NULL). C `partial 12.34`, balances 0.00/12.34, one transfer, `last_run_date` = job date. 2. **Apostrophe in the failure description** (database-backed): `fineract-provider/.../ExecuteStandingInstructionsHistoryPostgresTest#failureDescriptionWithAnApostropheIsStoredLiterallyAndTheNextInstructionStillPosts`. - Uses Testcontainers PostgreSQL with the real due query, the real history and `last_run_date` writes, and real transactions (the outer step transaction is simulated and rolls back on failure, as Spring Batch's does). - Only `transferFunds` is stubbed. It throws `Transfer rejected: customer's account is under review`. - Asserts the `failed` row stores that text literally, and that the funded instruction after it gets exactly one `success` row and posts. 3. **Exact attempted amount**: same class, `#failedRowRecordsTheExactAttemptedAmount`. - Partial transfers are off. Asserts the instruction amount is `9000000000000.000001` before the run. - After the run, asserts exactly one `failed` row whose `amount`, read as `BigDecimal`, equals `9000000000000.000001`, and that both balances keep their opening values. 4. **Error text longer than `error_log`**: same class, `#failureDescriptionLongerThanTheErrorLogColumnStillProducesAFailedRow`. The stubbed transfer throws a message of more than 600 characters. Asserts exactly one `failed` row with the attempted amount, an `error_log` of exactly 500 characters that keeps the start of the message, untouched balances, and that the funded instruction after it still posts one `success` row. #### Fidelity of the hand-written test schema The Testcontainers test creates only the tables and columns the due query and the job touch. I compared them with the schema Liquibase actually produces, read from `information_schema` of a freshly migrated `fineract_default` on PostgreSQL. Every column of `m_account_transfer_standing_instructions` and `m_account_transfer_standing_instructions_history` matches in type, length, precision/scale, nullability and default. The differences, none of which the tests exercise: | Table.column | Test fixture | Liquibase | |---|---|---| | `m_client.display_name` | `varchar(100)` | `varchar(160)` (`0127_client_name_length.xml`) | | `m_account_transfer_details.from_client_id`, `to_client_id` | `NOT NULL` | nullable | | `m_savings_account.account_balance_derived` | no default | default `0` | | `id` columns | plain `bigint` primary key (ids supplied by the test); `bigserial` for the history table | `bigint GENERATED BY DEFAULT AS IDENTITY` | #### Each regression test fails on the original behaviour The database-backed tests were run against the 8e80aa6 tasklet: - the apostrophe test fails with `BadSqlGrammarException: Unterminated string literal`; - the exact-amount test fails with 0 history rows, because the failed row was rolled back with the step; - the long-error-text test fails with `DataIntegrityViolationException: value too long for type character varying(500)`. The mixed-outcome IT was run against a server built with **only** `ExecuteStandingInstructionsTasklet` and `ExecuteStandingInstructionsConfig` reverted to 8e80aa6. The `lastRunDate` field and `FeignSchedulerHelper` change were kept so that the test compiles. Result: - The `job_run_history` assertion (`status == "failed"`) passes. - The next assertion fails: ``` StandingInstructionMixedOutcomeTest.singleNewHistoryRow(StandingInstructionMixedOutcomeTest.java:167) AssertionFailedError: history of standing instruction 14: [] ==> expected: <1> but was: <0> ``` Instruction 14 is A, the funded instruction. B's failure rolled back the shared step transaction, which discarded A's transfer and its `success` history row. Existing unit tests that captured the concatenated SQL string now read the bound status parameter instead. No assertion was removed or weakened. ### How to run ```bash # Unit + Testcontainers PostgreSQL tests (needs Docker; skipped, not failed, without it) ./gradlew :fineract-provider:test --tests 'org.apache.fineract.portfolio.account.*' --rerun-tasks # PostgreSQL integration test, using the documented Docker stack (CONTRIBUTING.md, "Integration tests") ./gradlew :fineract-provider:jibDockerBuild -Djib.to.image=fineract -x test -x cucumber docker compose -f docker-compose-postgresql-test.yml up -d ./gradlew :integration-tests:waitForFineract ./gradlew :integration-tests:test --tests 'org.apache.fineract.integrationtests.savings.StandingInstructionMixedOutcomeTest' # Related regression suites ./gradlew :integration-tests:test --tests '*StandingInstruction*' --tests '*Savings*' \ --tests 'org.apache.fineract.integrationtests.savings.*' --tests '*SchedulerJob*' ``` Results locally on PostgreSQL: - `org.apache.fineract.portfolio.account.*`: 62 passed, 0 skipped (`ExecuteStandingInstructionsHistoryPostgresTest` 3 of 3). - `StandingInstructionMixedOutcomeTest`: passed (with this fix); fails on the 8e80aa6 tasklet as shown above. - Related IT suites: 49 classes, 229 tests, all passed. - `:fineract-provider:cucumber`: 150 scenarios passed. - `spotlessCheck` and Checkstyle (`fineract-provider` main and test, `integration-tests` test): clean. ## Checklist Please make sure these boxes are checked before submitting your pull request - thanks! - [ ] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests) - [x] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers. - [x] Create/update [unit or integration tests](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made. - [x] Follow our [coding conventions](https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions). - [x] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes - [x] [This PR must not be a "code dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute). - [x] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately. - [x] I followed the [AI Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy). Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide). AI assistance: Claude Code (claude-opus-5-5), disclosed via the Assisted-By trailer per the AI Policy. -- 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]
