[WIP] - Fix #809: Allow time in start and end date

mayank8318 commented 4 years ago

Closes #809 Still WIP Facing a problem right now because the whole system is built around only using dates(just like the UI). This PR will require some major changes according to me so I want to clarify the design before going forward. The main culprit is the DateRange API which is used by the export tasks. It uses only LocalDate. So here are a few options I could think of: i) Implement it using a Generic <T extends ...> ii) Make a LocalDateTime API without touching this one iii) Change this to use LocalDateTime (lot of refactoring needed including the tests)

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

CC - @ggalmazor

ggalmazor commented 4 years ago

I agree with the assessment of the problem, @mayank8318. Thanks for pointing it out.

This change was intended to support the needs of some high-load operations that required fine-tuning the export filters. Now there's a new --smart_append feature that lets users do something similar.

Since the change will require bigger changes than anticipated, and considering the alternative --smart_append workaround, I wonder if we should really go ahead with this change.

Having said that, I think we should go with OffsetDateTime values anyway, because users might want to use time zones different than the local ones.

Regarding strategy and design, here's how we could do the change using parallel-refactoring:

Rename DateRange into LocalDateRange
Extract a DateRange interface from LocalDateRange
Make all the code depend on DateRange instead of LocalDateRange
Duplicate LocalDateRange into OffsetDateTimeRange and start refactoring that one with the help of the tests. Write more tests if needed to work out new behavior.
At CLI operation level, use a different subclass of DateRange according to what the user provides.
At UI operation level, always use LocalDateRange
Create a test that verifies relationships between date and dateTime ranges: provided that the system is in UTC, verify that start at 2019-01-01 is the same as start at 2019-01-01T00:00:00.000Z

ggalmazor commented 4 years ago

Hi, @mayank8318!

I'm not sure if you had a moment to think about my last comment. To be clear, I'm inclined to close this PR and do nothing about #809 for the moment. How does that sound?

mayank8318 commented 4 years ago

Hi @ggalmazor sorry I couldn't just glance at the comment but didn't get time to think about the implementation. Yes, I agree with you. I'll close the PR :)

getodk / briefcase