FINERACT-2701: support OffsetTime for savings transactions

[AnvayKharb]

Description

This PR adds support for an optional transactionTime field for savings transactions using OffsetTime while maintaining full backward compatibility with the existing API.

The existing transactionDate field remains unchanged. Clients can optionally provide transactionTime in ISO-8601 offset time format (for example, 14:30:00+05:30). The provided time is normalized to UTC before persistence.

The implementation also propagates the optional transaction time through account transfer flows to ensure consistent transaction handling across savings operations.

Changes

• Added optional transactionTime support using OffsetTime.
• Preserved backward compatibility for existing date-only requests.
• Added parsing and serialization for ISO-8601 offset time values.
• Normalized transaction times to UTC before persistence.
• Added database support for storing transaction time.
• Updated validation, domain models, DTOs, services, mappers, and API documentation.
• Added tests covering parsing, serialization, persistence, UTC normalization, and backward compatibility.

Related Issue

FINERACT-2701

[adamsaghy]

It looks weird at first...
Let me review a little deeper...

We should really not use LocalDateTime and mess with modifiers (Midnight...)

[adamsaghy]

1. I’m not keen on introducing a brand new field called TIMESTAMP (on the database side) and LocalDateTime (on the Java side). Instead, let’s use OffsetTime to ensure accurate timezone handling.

2. The transaction date field already exists in the savings transaction and savings transfer entities, and it’s of type DATE. Do we really need a user-provided time part? Will this be different from the created date time (OffsetDateTime)?

If we do need a user-provided time part, let’s add a new field that stores as OffsetTime and is optional for backward compatibility.

3. I suggest that we request the transaction date and transaction time as two separate fields. This way, we avoid supporting a single field with different data types.

4. Let’s avoid using LocalDateTime and parsing to that data type in all places. Instead, the user should provide the time part with an offset TZ. Is this a viable option?

@AnvayKharb @IOhacker what do you think?

[AnvayKharb]

Hi @adamsaghy Thanks for the detailed review. I understand your concerns. Using a separate optional transactionTime with timezone information while keeping the existing transactionDate sounds like a cleaner and more backward-compatible approach than introducing a LocalDateTime field. I'll wait for @IOhacker 's thoughts as well, and if we're aligned on this direction, I'll update the implementation accordingly.

[AnvayKharb]

Thanks for the guidance @adamsaghy @IOhacker . I updated the PR to follow the separate optional transaction time approach instead of the earlier combined date-time design.

Current implementation:

• Keeps the existing transaction date fields unchanged.
• Adds an optional separate transactionTime field using OffsetTime.
• Accepts ISO offset time values such as 14:30:00+05:30, 04:15:30-04:00, and 09:00:00Z.
• Normalizes provided transactionTime values to UTC before persistence.
• Leaves date-only requests fully backward compatible with transactionTime omitted/null.
• Uses PostgreSQL TIME WITH TIME ZONE and MySQL/MariaDB TIME(6).
• Removes the earlier combined transaction_datetime/TIMESTAMP/LocalDateTime approach.

I also rechecked the diff for newly introduced LocalDateTime, transaction_datetime, transactionDateTime, .atStartOfDay(), midnight conversions, and generic TIMESTAMP usage; none remain in this PR diff. Local validation passed for OpenAPI compatibility, formatting/checkstyle, focused transaction-time tests, Liquibase DDL safety, and Liquibase-only runs against both PostgreSQL and MariaDB with actual column type verification.

[adamsaghy]

I dont think we need this many options... the field is stored as OffsetTime and fetched as OffsetTime. No need for anything else but reading as OffsetTime...

[adamsaghy]

Please dont mark as resolved without explanation or changes!

[AnvayKharb]

Hi @adamsaghy I apologize for not explaining, I initially added the conversion to preserve the offset consistently, but since the field is already stored and fetched as OffsetTime, that extra logic isn't needed. I'll simplify it to direct OffsetTime handling.

[adamsaghy]

@AnvayKharb @IOhacker Can you help me understand this new field a little better?

Is this provided new field will be different then the "created date time"?

[AnvayKharb]

Hi @adamsaghy Yes, they're different. createdDateTime is the system timestamp when Fineract persisted the transaction, while transactionTime is the optional business time provided by the user for the transaction. I'll simplify the implementation as suggested by removing the converter/helper and persisting OffsetTime directly.

[adamsaghy]

    Did you mean 'private static  boolean urlFunction(String s){return LOAN_PATH_PATTERN.matcher(s).find();}'?
  Error: /home/runner/work/fineract/fineract/fineract-provider/src/main/java/org/apache/fineract/portfolio/account/domain/AccountTransferTransaction.java:177: error: [UnusedMethod] Constructor 'AccountTransferTransaction' is never used.
      private AccountTransferTransaction(final AccountTransferDetails accountTransferDetails, final SavingsAccountTransaction withdrawal,
              ^

Also please make sure your commits are signed by GPG

[adamsaghy]

@AnvayKharb Please review the failing checks.

[AnvayKharb]

Hi @adamsaghy @IOhacker all the checks are now passing

[adamsaghy]

@AnvayKharb Please review the below concerns:

Blocking

1. transactionDate + transactionTime can describe the wrong instant (day-boundary bug)

Every write path normalizes the time to UTC — fineract-savings/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountTransaction.java, fineract-provider/src/main/java/org/apache/fineract/portfolio/account/domain/AccountTransferTransaction.java, fineract-savings/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsAccountTransactionDTO.java, and again in JsonParserHelper.extractOffsetTimeNamed — but transactionDate is still taken verbatim from the client's local calendar.

POST { "transactionDate": "27 July 2026", "transactionTime": "02:00:00+05:30" }
stored: date = 2026-07-27, time = 20:30:00Z

Combining those, as your own review comment asked for ("transaction date + transaction time together can represent a more precise point in time"), gives 2026-07-27T20:30Z. The actual instant was 2026-07-26T20:30Z — off by a full day. Same class of error for any time whose UTC conversion crosses midnight in either direction.

The fix is to stop normalizing and persist the OffsetTime exactly as supplied. timetz keeps the offset, so 2026-07-27 + 02:00:00+05:30 reconstructs the correct instant. Normalizing is what breaks it, and it also makes the round-trip lossy (the client can no longer see the offset they sent).

Caveat that pushes back on the design: MySQL's TIME(6) cannot store an offset at all, so "stored as OffsetTime, fetched as OffsetTime" only holds on PostgreSQL. On MySQL Hibernate falls back to NORMALIZE against the JVM default zone. Fineract forces -Duser.timezone=UTC in its run configs so it works today, but it's implicit — worth setting hibernate.timezone.default_storage=NORMALIZE_UTC if you keep MySQL support here.

2. template=true silently drops the persisted time

SavingsAccountTransactionData.templateOnTop() (fineract-core/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsAccountTransactionData.java) rebuilds the object via createData(...) and never copies transactionTime. So GET /v1/savingsaccounts/{id}/transactions/{txnId}?template=true returns transactionTime: null for a transaction that has one (fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/api/SavingsAccountTransactionsApiResource.java). This is a direct consequence of the withTransactionTime mutator approach — a builder (as you already suggested) wouldn't have this hole.

3. Transaction adjustment accepts transactionTime and ignores it

SavingsAccountTransactionDataValidator.validate() now whitelists transactionTime, and adjustSavingsTransaction (fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsAccountWritePlatformServiceJpaRepositoryImpl.java) calls that same validator — but builds its SavingsAccountTransactionDTO without the time. Adjusting a transaction therefore accepts the parameter, drops it, and returns 200. Either wire it through or reject it on that endpoint.

Unaddressed from your earlier review

• JdbcSupport.getOffsetTime still has all three branches (OffsetTime / LocalTime / java.sql.Time). You asked for direct OffsetTime reading and the author agreed to simplify it on 2026-09-07; it hasn't been done.
• @Schema annotations are still on AccountTransferData.transferTime and SavingsAccountTransactionData.transactionTime, which you said weren't needed.

Non-blocking, but still should be review and address

• fields= filtering — transactionTime wasn't added to SAVINGS_TRANSACTION_RESPONSE_DATA_PARAMETERS (fineract-savings/src/main/java/org/apache/fineract/portfolio/savings/api/SavingsApiSetConstants.java), so ?fields=transactionTime throws UnsupportedParameterException. Normal responses are unaffected.
• Validation style — both validators call extractOffsetTimeNamed(...) purely for its throwing side effect and discard the result. A malformed time throws PlatformApiDataValidationException immediately instead of accumulating into dataValidationErrors, so it masks every other validation error in the same request. Every neighbouring check uses baseDataValidator.reset().parameter(...).
• Liquibase — the repo convention is one with a portable type (see TIMESTAMP WITH TIME ZONE in 0136/0162/0230); this splits into a Postgres plus a raw MySQL block with no rollback and no preconditions. Only 2 of 254 part files use dbms at all.
• Deposit accounts — DepositAccountReadPlatformServiceImpl:1130 maps the same m_savings_account_transaction rows but wasn't updated, so fixed/recurring deposit transaction endpoints won't return transactionTime. Fine if intentionally out of scope, but worth stating.
• Converter inconsistency — OffsetTimeJsonConverter.convertToObject does not normalize to UTC while JsonParserHelper.extractOffsetTimeNamed does. Harmless today (no Jackson-bound OffsetTime input exists), but a trap later.

[AnvayKharb]

@adamsaghy
Resolved the blocking review findings in commit dd2ee740000d835ced6b73ec258a975268d10bf7.

• Preserved the supplied OffsetTime on PostgreSQL, preventing UTC day-boundary errors.
• Preserved transactionTime for template=true.
• Wired transactionTime through transaction adjustments.
• Added fixed/recurring deposit and fields=transactionTime support.
• Updated validation, JDBC handling, and the portable Liquibase migration.
• Added end-to-end coverage for deposits, adjustments, transfers, templates, field filtering, and missing time values.