FINERACT-826 Migrate to java.time from Joda API

[percyashu]

Description

Describe the changes made and why they were made. Ignore if these details are present on the associated Jira ticket https://issues.apache.org/jira/browse/FINERACT-826

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.

• Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.

• API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm has been updated with details of any API changes.

• Integration tests have been created/updated for verifying the changes made.

• All Integrations tests are passing with the new commits.

• Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the list.)

Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide

[awasum]

@percyashu do you plan on working this one along side the blocker issues? If you can manage of that, then its Ok with me. Its also Ok to take one at a time and complete it or handling 2 issues at a time when you are stalk on one. Lets just make sure we finish what we start.

[awasum]

Did you try to run this locally before pushing? There is spotless violation.

[percyashu]

I plan on working on this with blocker issues.
And I didn't run it since there is still much to modify before it is ready.

[awasum]

Thats Ok to work on them side by side. I will encourage you to always run the build locally so that anyone reviewing should not have to stop at spotless failures given that the actual changes won't even be tested by Travis if spotless is not happy. Keep up the good work.

@vorburger , you might want to keep an eye on this over the next few days and week as this matures. I feel you have more experience on this given your comment on this on Jira. CC @ptuomola , @xurror , @thesmallstar .

[ptuomola]

Hi - in all the places where this currently calls ZoneId.systemDefault(), should we not be retrieving the appropriate tenant timezone instead? I know that's not what the previous code does, but wouldn't that be the right behaviour for any logic that is related to a specific tenant - i.e. we should be using the tenant's timezone for any dates?

[vorburger]

@percyashu perhaps you would like to split what @ptuomola is suggesting into a new JIRA issue and PR? So we could FIRST do an exact "like for like" migration (purely technical API change), and THEN look at switching to correct Tenant aware use?

[percyashu]

Ok will create a new issue to handle replacing ZoneId.systemDefault() to appropriate tenant timezone.

[percyashu]

@ptuomola @vorburger could have a look at this I have some difficulty finding out what cause the build to fail.

[xurror]

The error you have looks alot like the ones I'm facing with JPA migration. I suspect something is wrong at the level of persisting data. Probably an SQL violation or something similar.
I don't know about the joda time thing but you can start by checking the type of the attribute that is getting persisted with the previous one.

[percyashu]

CampaignsTest > testSupportedActionsForCampaignWithTriggerTypeAsScheduled() nows fail and I cant find why.
I succeeded in activating a campaign using the same endpoint as that of the test and it returns the ID as the test requires.
@ptuomola @vorburger @awasum your assistance will be greatly appreciated.

[percyashu]

@ptuomola @vorburger I think this is ready for a review once build passes.

[vorburger]

I've not reviewed this, but in the end are you disabling any tests, or are they all passing now?

[percyashu]

I've not reviewed this, but in the end are you disabling any tests, or are they all passing now?

I have disabled one test which is still failing. Will dig more into them

[percyashu]

@vorburger @awasum @ptuomola Should I create a jira issue for the remaining test since it is proving quite difficult to reproduce.

[vorburger]

@vorburger @awasum @ptuomola Should I create a jira issue for the remaining test since it is proving quite difficult to reproduce.

Hm. I'm quite torn how to proceed here. In an ideal world, it be neat if we figured out what is causing that test to still fail, and fix that as part as that PR before merging this. @ptuomola you actually probably already have the most experience in this area - is this somewhat similar to those flaky ITs you had managed to stabilize? If we absolutely cannot... hm. Should we still get this into a possible upcoming 1.4.0, or... hold merging this back to avoid any "risk", and make this a future 1.5.0 goal?

[ptuomola]

@vorburger I agree we should understand why tests are failing before merging this - if they are indications of some remaining issues that haven't been found, then the impact of those issues could be significant.

Maybe one idea would be to try to now stabilise and finalise 1.4.0 to get it out of the door, and after that start development on 1.5.0 by merging all the more significant / risky changes (i.e. this and the EclipseLink migraition)...

[percyashu]

it be neat if we figured out what is causing that test to still fail, and fix that as part as that PR before merging this.

Finally was able to reproduce the test and resolved it. No test was disabled in this PR.

[percyashu]

@awasum @vorburger @ptuomola waiting for your final review for this so it can be merged.

[vorburger]

@ptuomola do you want to review and merge this (big) one, some time? Just asking, and to avoid double reviews.

[ptuomola]

@vorburger I wouldn't claim to be an expert on the Java / Joda APIs, but I'm happy to have a look - won't be until this weekend though as I'm a bit snowed under with work! If you'd like to review in the meantime please just go ahead...

[awasum]

Woow, this is a big PR. this definitely needs multiple reviewers. I will ask on the mailing list. I dont have a lot of experience in this area.

[vorburger]

@ptuomola did you still want to review this PR one of these days?

[ptuomola]

@vorburger I left some review comments but I'm assuming you can't see them as I've left the review in "pending" state. My bad.

What I got stuck on was the handling for date formatting. Looks like java.time is stricter on date format strings: "dd MMM yyyy" that we use means the date has to have two digits i.e. leading zero for 1-9, whereas Joda accepted a single digit as well. The PR fixes this by changing every date in the tests to be 01 etc. But my concern was about the real clients out there calling the APIs: will this not break the API for all of them, as they have been sending dates without leading zeros?

The right solution of course would be to use the right date format "d M y" which accepts both leading zero and without. But the date format is a string that is sent by the client in some cases. And we have no control over what format strings they send...

So I wasn't able to find a solution that we could use that would avoid breaking API clients. Any ideas would be very welcome...

[percyashu]

@ptuomola Yeah java.time is stricter on date format, I noticed this during the migration.

The PR fixes this by changing every date in the tests to be 01 etc

Actually the PR doesn't fixes this in that way. I resolved the issue by making use of parseLenient(). So even with 0 the date can still be parsed.
I just added the 0 since they weren't that many in the ITs.

[ptuomola]

Actually the PR doesn't fixes this in that way. I resolved the issue by making use of parseLenient(). So even with 0 the date can still be parsed.
I just added the 0 since they weren't that many in the ITs.

Great stuff! I didn't get that far in my review - just saw you had changed all the dates in the ITs, and started wondering how we would manage that change wrt the existing clients!

Now with that one solved, I will take a closer look at the rest of the code later this week... sorry about the delay.

[ptuomola]

Finally had a chance to go through all the changed files.

They look OK to me, so I'd support merging this. But would be good if someone else could also take a look? @vorburger @xurror @awasum?

Also there are some merge conflicts that now need to be resolved

[vorburger]

@awasum remind me who was the Mentor ;) of @percyashu during GSOC? Perhaps that person could do the final review of mentee work, and merge this if LGTY?

[awasum]

@ptuomola and @vorburger . Thanks for reviewing. I will take a closer look tomorrow and probably merge this one. This is great work @percyashu . Do we need to write and inform the rest of the community on the mailing list? (I think its worth it, this is massive). Next is to handle https://issues.apache.org/jira/browse/FINERACT-1112 as a follow up to this. Anyone going to handle that one after this is merged?

[awasum]

LGTM. After merging this, https://issues.apache.org/jira/browse/FINERACT-1112 is then ready for someone to implement. @percyashu do you want to do that?

[avikganguly01]

@ptuomola @percyashu @awasum @vorburger :

FINERACT-826
actualDisbursementDate.toDate() got replaced by
java.util.Date.from(actualDisbursementDate.atStartOfDay(ZoneId.systemDefault()).toInstant());

Before FINERACT-1112
actualDisbursementDate = T
Server Timezone : +05:30Z, Tenant Timezone : +6Z
ZoneId zone = ZoneId.systemDefault(); //+05:30Z
ZonedDateTime dateTime = actualDisbursementDate.atStartOfDay(zone); //Midnight +5:30Z
Instant instant = dateTime.toInstant(); // T-1 Midnight + 18:30 0Z
Date dt = Date.from(instant); // T-1 Midnight + 18:30 + 5:30 = T 00:00 +05:30Z

FINERACT-1112
If tenant timezone is ahead of server timezone, then :-

Server Timezone : +05:30Z, Tenant Timezone : +6Z
actualDisbursementDate = T
ZoneId zone = DateUtils.getDateTimeZoneOfTenant(); //+6Z // Previously ZoneId.systemDefault()
ZonedDateTime dateTime = actualDisbursementDate.atStartOfDay(zone); //Midnight +6Z
Instant instant = dateTime.toInstant(); // T-1 Midnight + 18 0Z
Date dt = Date.from(instant); // T-1 Midnight + 18 + 5:30 = T-1 23:30 +05:30Z

If tenant timezone and server timezones are different, then this creates a T-1 issue for all txn dates populated by application.

Hi - in all the places where this currently calls ZoneId.systemDefault(), should we not be retrieving the appropriate tenant timezone instead? I know that's not what the previous code does, but wouldn't that be the right behaviour for any logic that is related to a specific tenant - i.e. we should be using the tenant's timezone for any dates?

actualDisbursementDate.toDate() got replaced by
java.util.Date.from(actualDisbursementDate.atStartOfDay(ZoneId.systemDefault()).toInstant());

This part looks mostly correct as this is the most popular way (https://stackoverflow.com/questions/22929237/convert-java-time-localdate-into-java-util-date-type/22929420) to convert time.date to util.date. Note that the popular answer is questioned in it's comments section because atStartOfDay changes the time but overall the answer fixes the question.

Problem is with using DateUtils.getDateTimeZoneOfTenant() instead of ZoneId.systemDefault() as this deviates from the solution provided for Fineract-826. A tenant is already passing the correct tenant-specific date in the API. There is no need to mess with the date by applying tenant timezone all over again.

We have reverted DateUtils.getDateTimeZoneOfTenant() to ZoneId.systemDefault() wherever date was being parsed with atStartOfDay logic and so far SIT is successful. Please approve the corresponding PR if you approve of this logic reversal.