[FINERACT-826] Migrate to java.time from Joda API

• Type: Bug
• Status: Resolved
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 1.4.0
• Fix Version/s: 1.5.0
• Labels: technical
• Assignee: Percy Ashu
• Reporter: Michael Vorburger
• Created: 12/Jan/20 13:05
• Updated: 07/Sep/21 12:51
• Resolved: 25/Oct/20 07:29

Description

http://mail-archives.apache.org/mod_mbox/fineract-dev/201907.mbox/[e-mail]%3e, and the original previous and the follow up replies on that thread Subject "Questions about my approach to find problems with tenant time zones, and about the instance values of SQL migration files.", related to FINERACT-723? I think, point out that org.apache.fineract.infrastructure.core.service.DateUtils could do with a closer look and fixes, specifically:

That DateUtils class is interesting.. do we really need to mix the old java.util.Date API and the org.joda.time Time API ? FYI Joda Time has been integrated into Java 8 as java.time. So if someone was up for taking up work related to cleaning that up, that could be useful IMHO (so converge everything to java.time, and eventually remove the Joda dependency, and introduce an enforced check via Checkstle or SpotBugs to forbid java.util.Date).
 
The null management in DateUtils is pretty interesting as well. So is this per tenant TZ guaranteed to always be in the DB? What does the DB schema say - required, never NULL? Does the Java code ensure it's never null? I'm asking these questions because something like that getLocalDateTimeOfTenant(), despite its name, seems to be actually be written to EITHER give us a "local" time based on the tenant, or, if that's null, it just falls back to what l suspect (not verified) is dependent on the OS / JVM TZ - that's scary. Perhaps it's not a problem in reality, if every tenant is guaranteed to always have a TZ, but if someone could double check this, and if so remove those null check and system dependent fallbacks, then IMHO that would make that utility code clearer.
 
BTW line 45 in DateUtils seems completely useless, agreed? Would you like to raise a PR proposing to ditch that?

Issue Links

• Blocked: FINERACT-1305 Release Apache Fineract v1.5.0
• is duplicated by: FINERACT-1258 NPE at LoanAccountDomainServiceJpa.recalculateAccruals()
• is related to: FINERACT-1075 Error Prone: Enable JdkObsolete Check
• is related to: FINERACT-1112 Replace ZoneId.systemDefault() with tenant's timezone
• relates to: FINERACT-1394 Partially Revert FIN-1112 "Replace ZoneId.systemDefault() with tenant's timezone"
• relates to: FINERACT-1242 OfficeTest fails with java.time.format.DateTimeParseException: Text '2020-10-25' could not be parsed: Unable to obtain LocalDateTime from TemporalAccessor: {DayOfMonth=25, Year=2020},ISO resolved to 00:10 of type java.time.format.Parsed
• links to: GitHub Pull Request #755
• links to: GitHub Pull Request #1179
• links to: GitHub Pull Request #1496