FINERACT-2701: support OffsetTime for savings transactions by AnvayKharb · Pull Request #6187 · apache/fineract

AnvayKharb · 2026-07-27T09:28:08Z

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 · 2026-07-29T03:49:33Z

+<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.3.xsd">
+    <changeSet author="fineract" id="1">


Please dont use TIMESTAMP... use TIMESTAMP WITH TZ for postgres and DATETIME(6) for mysql

adamsaghy · 2026-07-29T03:51:50Z

+        <addColumn tableName="m_savings_account_transaction">
+            <column name="transaction_datetime" type="TIMESTAMP"/>
+        </addColumn>
+        <addColumn tableName="m_account_transfer_transaction">


We already have transaction date for transfer and for savings transactions.

If the plan is to store a user provided TIME part as well, lets do it with timezone infomation and as a standalone field for the TIME only. transaction date + transaction time together can represent a more precise point in time... TZ informations are important to ensure correctness across timezones.

We are storing all date time and time in UTC.

adamsaghy

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.
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.

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.
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 · 2026-07-29T05:40:10Z

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 · 2026-07-31T09:10:22Z

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 · 2026-08-03T14:33:38Z

        return localTime;
    }

+    public static OffsetTime getOffsetTime(final ResultSet rs, final String columnName) throws SQLException {


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 · 2026-08-03T14:34:08Z

+import org.apache.fineract.infrastructure.core.service.DateUtils;
+
+@Converter
+public class UtcOffsetTimeAttributeConverter implements AttributeConverter<OffsetTime, LocalTime> {


We dont need this. We dont want to work with LocalTime...

adamsaghy · 2026-08-03T14:34:40Z

        return OffsetDateTime.now(ZoneOffset.UTC);
    }

+    public static OffsetTime toUtcOffsetTime(final OffsetTime time) {


No need for this...

adamsaghy · 2026-08-03T14:36:34Z

    private final CurrencyData currency;
    private final BigDecimal transferAmount;
    private final LocalDate transferDate;
+    @Schema(type = "string", example = "09:00:00Z", nullable = true)


I dont think we need swagger annotations here.

adamsaghy · 2026-08-03T14:36:38Z

    private final String accountNo;
    private final ExternalId externalId;
    private final LocalDate date;
+    @Schema(type = "string", example = "09:00:00Z", nullable = true)


I dont think we need swagger annotations here.

adamsaghy · 2026-08-03T14:37:15Z

                false);
    }

+    public static SavingsAccountTransactionData create(final Long id, final SavingsAccountTransactionEnumData transactionType,


Instead of creating the N+1 constructor, would you be open to rewrite these using builder pattern instead?

adamsaghy · 2026-08-03T14:37:41Z

    private LocalDate date;

+    @Column(name = "transaction_time")
+    @Convert(converter = UtcOffsetTimeAttributeConverter.class)


We dont need converter...

adamsaghy · 2026-08-03T14:40:07Z

+        validate(command, false);
+    }
+
+    public void validate(final JsonCommand command, final boolean transactionTimeSupported) {


I dont think we need final boolean transactionTimeSupported arg... since this field is added, it can be freely provided, no?

adamsaghy · 2026-08-03T14:42:24Z

@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 · 2026-08-03T16:53:03Z

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.

…aldatetime

AnvayKharb force-pushed the fix/FINERACT-2701-savings-transaction-localdatetime branch 4 times, most recently from c62facb to 7215a56 Compare July 27, 2026 18:46

IOhacker requested a review from adamsaghy July 28, 2026 16:26

IOhacker previously approved these changes Jul 28, 2026

View reviewed changes

adamsaghy requested changes Jul 28, 2026

View reviewed changes

adamsaghy reviewed Jul 29, 2026

View reviewed changes

adamsaghy requested changes Jul 29, 2026

View reviewed changes

AnvayKharb marked this pull request as draft July 29, 2026 19:46

AnvayKharb dismissed IOhacker’s stale review via e269bd1 July 29, 2026 19:52

AnvayKharb force-pushed the fix/FINERACT-2701-savings-transaction-localdatetime branch 3 times, most recently from 587fa6b to 287c73f Compare July 31, 2026 09:05

AnvayKharb marked this pull request as ready for review July 31, 2026 09:10

AnvayKharb requested review from IOhacker and adamsaghy July 31, 2026 09:13

IOhacker changed the title ~~FINERACT-2701: support LocalDateTime for savings transactions~~ FINERACT-2701: Support OffsetTime for savings transactions Jul 31, 2026

AnvayKharb changed the title ~~FINERACT-2701: Support OffsetTime for savings transactions~~ fix(FINERACT-2701): support OffsetTime for savings transactions Jul 31, 2026

AnvayKharb changed the title ~~fix(FINERACT-2701): support OffsetTime for savings transactions~~ FINERACT-2701: support OffsetTime for savings transactions Aug 1, 2026

AnvayKharb force-pushed the fix/FINERACT-2701-savings-transaction-localdatetime branch from 287c73f to cb7bd37 Compare August 1, 2026 08:34

adamsaghy reviewed Aug 3, 2026

View reviewed changes

fix(FINERACT-2701): support transaction time for savings transactions

59ab286

AnvayKharb force-pushed the fix/FINERACT-2701-savings-transaction-localdatetime branch from cb7bd37 to 59ab286 Compare August 3, 2026 17:14

AnvayKharb requested a review from adamsaghy August 3, 2026 17:19

Merge branch 'develop' into fix/FINERACT-2701-savings-transaction-loc…

cff0e0d

…aldatetime

AnvayKharb force-pushed the fix/FINERACT-2701-savings-transaction-localdatetime branch from 16f06b0 to cff0e0d Compare August 3, 2026 18:13

Uh oh!

Conversation

AnvayKharb commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Related Issue

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

AnvayKharb commented Jul 29, 2026

Uh oh!

AnvayKharb commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy commented Aug 3, 2026

Uh oh!

AnvayKharb commented Aug 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AnvayKharb commented Jul 27, 2026 •

edited

Loading

AnvayKharb commented Jul 31, 2026 •

edited

Loading