FINERACT-2701: support LocalDateTime for savings transactions - #6187
FINERACT-2701: support LocalDateTime for savings transactions#6187AnvayKharb wants to merge 1 commit into
Conversation
61a46fe to
c62facb
Compare
c62facb to
7215a56
Compare
adamsaghy
left a comment
adamsaghy
left a comment
There was a problem hiding this comment.
It looks weird at first...
Let me review a little deeper...
We should really not use LocalDateTime and mess with modifiers (Midnight...)
| <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"> |
There was a problem hiding this comment.
Please dont use TIMESTAMP... use TIMESTAMP WITH TZ for postgres and DATETIME(6) for mysql
| <addColumn tableName="m_savings_account_transaction"> | ||
| <column name="transaction_datetime" type="TIMESTAMP"/> | ||
| </addColumn> | ||
| <addColumn tableName="m_account_transfer_transaction"> |
There was a problem hiding this comment.
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
left a comment
adamsaghy
left a comment
There was a problem hiding this comment.
-
I’m not keen on introducing a brand new field called
TIMESTAMP(on the database side) andLocalDateTime(on the Java side). Instead, let’s useOffsetTimeto ensure accurate timezone handling. -
The
transaction datefield already exists in thesavings transactionandsavings transferentities, and it’s of typeDATE. 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
LocalDateTimeand 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?
|
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. |
3 participants
Description
This PR adds support for
LocalDateTimein Savings Account withdrawals and transfers while maintaining full backward compatibility with the existingLocalDaterequest format.The implementation allows clients to continue sending
yyyy-MM-ddrequests unchanged while also acceptingyyyy-MM-dd'T'HH:mm:ssfor improved transaction date and time precision. Existing behavior for date-only requests is preserved, and theUnsupportedTemporalTypeExceptionencountered when formatting transaction dates with time precision has been resolved.Related Issues
Closes #FINERACT-2701