Bug 24850 - Koha::DateUtils ignores offsets in RFC3339 datetimes
Summary: Koha::DateUtils ignores offsets in RFC3339 datetimes
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 24643
Blocks: 24609 29401 29402 28377 29403 29906
  Show dependency treegraph
 
Reported: 2020-03-11 15:23 UTC by Martin Renvoize
Modified: 2023-06-08 22:26 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Prior to this patch our date handling library ignored offset data passed with rfc3339 dates. This could lead to problems if an API client converted to UTC or was in a different timezone to the Koha instance time setting. This patch adds proper handling to dt_from_string such that if an REF3339 date is input, we parse out the offset and then adjust the time to match the instance timezone for storage.
Version(s) released in:
21.11.00


Attachments
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.38 KB, patch)
2020-03-11 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (1.99 KB, patch)
2020-03-11 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.35 KB, patch)
2020-03-11 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.38 KB, patch)
2020-03-11 17:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (1.99 KB, patch)
2020-03-11 17:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.33 KB, patch)
2020-03-11 17:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Don't fallback when rfc3339 format is passed (1.46 KB, patch)
2020-03-11 17:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.38 KB, patch)
2020-03-13 12:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (1.99 KB, patch)
2020-03-13 12:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.33 KB, patch)
2020-03-13 12:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Don't fallback when date_format is passed (1.71 KB, patch)
2020-03-13 12:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Always return 'instance' local time (1.07 KB, patch)
2020-03-17 10:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.38 KB, patch)
2020-03-17 14:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (1.99 KB, patch)
2020-03-17 14:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.33 KB, patch)
2020-03-17 14:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Don't fallback when date_format is passed (1.71 KB, patch)
2020-03-17 14:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Always return 'instance' local time (1.85 KB, patch)
2020-03-17 14:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.38 KB, patch)
2020-03-26 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (1.99 KB, patch)
2020-03-26 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.33 KB, patch)
2020-03-26 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Don't fallback when date_format is passed (1.71 KB, patch)
2020-03-26 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Always return 'instance' local time (1.85 KB, patch)
2020-03-26 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Continue to handle default time and daylight saving (1.71 KB, patch)
2020-03-26 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.43 KB, patch)
2020-03-26 13:43 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.43 KB, patch)
2020-03-26 13:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.43 KB, patch)
2020-03-26 13:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (2.04 KB, patch)
2020-03-26 13:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.38 KB, patch)
2020-03-26 13:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Don't fallback when date_format is passed (1.76 KB, patch)
2020-03-26 13:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Always return 'instance' local time (1.90 KB, patch)
2020-03-26 13:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Continue to handle default time and daylight saving (1.77 KB, patch)
2020-03-26 13:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.38 KB, patch)
2021-01-27 08:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (1.98 KB, patch)
2021-01-27 08:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.33 KB, patch)
2021-01-27 08:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Don't fallback when date_format is passed (1.71 KB, patch)
2021-01-27 08:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Always return 'instance' local time (1.84 KB, patch)
2021-01-27 08:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Continue to handle default time and daylight saving (1.77 KB, patch)
2021-01-27 08:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: (QA follow-up) Add more comments (1001 bytes, patch)
2021-01-27 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.43 KB, patch)
2021-02-04 03:51 UTC, David Cook
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (2.03 KB, patch)
2021-02-04 03:51 UTC, David Cook
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.38 KB, patch)
2021-02-04 03:51 UTC, David Cook
Details | Diff | Splinter Review
Bug 24850: Don't fallback when date_format is passed (1.76 KB, patch)
2021-02-04 03:51 UTC, David Cook
Details | Diff | Splinter Review
Bug 24850: Always return 'instance' local time (1.89 KB, patch)
2021-02-04 03:51 UTC, David Cook
Details | Diff | Splinter Review
Bug 24850: Continue to handle default time and daylight saving (1.82 KB, patch)
2021-02-04 03:51 UTC, David Cook
Details | Diff | Splinter Review
Bug 24850: (QA follow-up) Add more comments (1.03 KB, patch)
2021-02-04 03:51 UTC, David Cook
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.53 KB, patch)
2021-02-05 08:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (2.13 KB, patch)
2021-02-05 08:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.47 KB, patch)
2021-02-05 08:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24850: Don't fallback when date_format is passed (1.85 KB, patch)
2021-02-05 08:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24850: Always return 'instance' local time (1.99 KB, patch)
2021-02-05 08:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24850: Continue to handle default time and daylight saving (1.92 KB, patch)
2021-02-05 08:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24850: (QA follow-up) Add more comments (1.13 KB, patch)
2021-02-05 08:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24850: (follow-up) Add tests for invalid RFC3339 dates (1.23 KB, patch)
2021-02-08 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.49 KB, patch)
2021-10-13 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (2.09 KB, patch)
2021-10-13 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.43 KB, patch)
2021-10-13 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Don't fallback when date_format is passed (1.81 KB, patch)
2021-10-13 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Always return 'instance' local time (1.95 KB, patch)
2021-10-13 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Continue to handle default time and daylight saving (1.88 KB, patch)
2021-10-13 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: (QA follow-up) Add more comments (1.09 KB, patch)
2021-10-13 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: (follow-up) Add tests for invalid RFC3339 dates (1.23 KB, patch)
2021-10-13 12:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Date only are not RFC3339 (1.14 KB, patch)
2021-11-03 09:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24850: Date only are not RFC3339 (1.14 KB, patch)
2021-11-03 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24850: Fix api/v1/holds.t (6.12 KB, patch)
2021-11-03 09:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24850: suspended_until is a date-time (900 bytes, patch)
2021-11-03 09:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24850: Date only are not RFC3339 (1.15 KB, patch)
2021-11-03 11:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24850: Correctly handle suspended_until for PUT (1.61 KB, patch)
2021-11-03 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24850: Fix api/v1/holds.t (6.17 KB, patch)
2021-11-03 11:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: suspended_until is a date-time (965 bytes, patch)
2021-11-03 11:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Date only are not RFC3339 (1.21 KB, patch)
2021-11-03 11:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correctly handle suspended_until for PUT (1.67 KB, patch)
2021-11-03 11:39 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-03-11 15:23:15 UTC
The final test added in bug 24643 incorrectly tests RFC3339 timezone handling.

If an offset is passed it should be applied to the given time to return an epoch relative to UTC and then the koha timezone config value should be applied against that to give us a koha instance relative time.
Comment 1 Martin Renvoize 2020-03-11 16:22:31 UTC
Created attachment 100585 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff
Comment 2 Martin Renvoize 2020-03-11 16:22:34 UTC
Created attachment 100586 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.
Comment 3 Martin Renvoize 2020-03-11 16:22:36 UTC
Created attachment 100587 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.
Comment 4 Martin Renvoize 2020-03-11 17:56:52 UTC
Created attachment 100592 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff
Comment 5 Martin Renvoize 2020-03-11 17:56:55 UTC
Created attachment 100593 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.
Comment 6 Martin Renvoize 2020-03-11 17:56:57 UTC
Created attachment 100594 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.
Comment 7 Martin Renvoize 2020-03-11 17:57:00 UTC
Created attachment 100595 [details] [review]
Bug 24850: Don't fallback when rfc3339 format is passed
Comment 8 Martin Renvoize 2020-03-11 17:58:49 UTC
I'm not sure about the final followup.. it scares me that we're always falling back to a relatively loose regex at the moment; even if a date_format is passed in.
Comment 9 Martin Renvoize 2020-03-12 10:02:18 UTC
Finally.. I think we also need to add some handling into the DateTime::Format::MySQL datetime_parser that we are using via DBIx::Class.. it does not take into account offsets/timezones in the format_datetime method.
Comment 10 Martin Renvoize 2020-03-13 12:03:11 UTC
Created attachment 100678 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff
Comment 11 Martin Renvoize 2020-03-13 12:03:14 UTC
Created attachment 100679 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.
Comment 12 Martin Renvoize 2020-03-13 12:03:17 UTC
Created attachment 100680 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.
Comment 13 Martin Renvoize 2020-03-13 12:03:20 UTC
Created attachment 100681 [details] [review]
Bug 24850: Don't fallback when date_format is passed
Comment 14 Martin Renvoize 2020-03-13 12:04:27 UTC
(In reply to Martin Renvoize from comment #8)
> I'm not sure about the final followup.. it scares me that we're always
> falling back to a relatively loose regex at the moment; even if a
> date_format is passed in.

Corrected in final followup.
Comment 15 Martin Renvoize 2020-03-17 10:23:43 UTC
Created attachment 100881 [details] [review]
Bug 24850: Always return 'instance' local time

This patch adds handling to ensure we always convert a passed in time to
the instance configured timezone..
Comment 16 Martin Renvoize 2020-03-17 14:52:27 UTC
Created attachment 100901 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff
Comment 17 Martin Renvoize 2020-03-17 14:52:30 UTC
Created attachment 100902 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.
Comment 18 Martin Renvoize 2020-03-17 14:52:34 UTC
Created attachment 100903 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.
Comment 19 Martin Renvoize 2020-03-17 14:52:37 UTC
Created attachment 100904 [details] [review]
Bug 24850: Don't fallback when date_format is passed
Comment 20 Martin Renvoize 2020-03-17 14:52:40 UTC
Created attachment 100905 [details] [review]
Bug 24850: Always return 'instance' local time

This patch adds handling to ensure we always convert a passed in time to
the instance configured timezone..
Comment 21 Bernardo Gonzalez Kriegel 2020-03-23 20:30:04 UTC
Martin,
t/DateUtils.t fails always. 
An special setup is needed?
Comment 22 Martin Renvoize 2020-03-26 10:27:59 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #21)
> Martin,
> t/DateUtils.t fails always. 
> An special setup is needed?

Nope.. seems I forgot to check the tests again after than final patch.. looking at it now.
Comment 23 Martin Renvoize 2020-03-26 11:14:20 UTC
Created attachment 101803 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff
Comment 24 Martin Renvoize 2020-03-26 11:14:23 UTC
Created attachment 101804 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.
Comment 25 Martin Renvoize 2020-03-26 11:14:27 UTC
Created attachment 101805 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.
Comment 26 Martin Renvoize 2020-03-26 11:14:30 UTC
Created attachment 101806 [details] [review]
Bug 24850: Don't fallback when date_format is passed
Comment 27 Martin Renvoize 2020-03-26 11:14:33 UTC
Created attachment 101807 [details] [review]
Bug 24850: Always return 'instance' local time

This patch adds handling to ensure we always convert a passed in time to
the instance configured timezone..
Comment 28 Martin Renvoize 2020-03-26 11:14:36 UTC
Created attachment 101808 [details] [review]
Bug 24850: Continue to handle default time and daylight saving

This patch prevents the call to set_time_zone if we are handling a
dateonly datetime string or a DST time
Comment 29 Agustín Moyano 2020-03-26 13:43:00 UTC
Created attachment 101837 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 30 Agustín Moyano 2020-03-26 13:44:12 UTC
Created attachment 101838 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 31 Agustín Moyano 2020-03-26 13:45:17 UTC
Created attachment 101839 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 32 Agustín Moyano 2020-03-26 13:45:21 UTC
Created attachment 101840 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 33 Agustín Moyano 2020-03-26 13:45:25 UTC
Created attachment 101841 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 34 Agustín Moyano 2020-03-26 13:45:29 UTC
Created attachment 101842 [details] [review]
Bug 24850: Don't fallback when date_format is passed

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 35 Agustín Moyano 2020-03-26 13:45:32 UTC
Created attachment 101843 [details] [review]
Bug 24850: Always return 'instance' local time

This patch adds handling to ensure we always convert a passed in time to
the instance configured timezone..

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 36 Agustín Moyano 2020-03-26 13:45:36 UTC
Created attachment 101844 [details] [review]
Bug 24850: Continue to handle default time and daylight saving

This patch prevents the call to set_time_zone if we are handling a
dateonly datetime string or a DST time

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 37 Jonathan Druart 2020-04-08 10:58:53 UTC
+        # Default to UTC (when 'Z' is passed) for inbound timezone.

Do you have an example where this "Z" is passed?
Comment 38 Martin Renvoize 2020-04-15 07:53:10 UTC
(In reply to Jonathan Druart from comment #37)
> +        # Default to UTC (when 'Z' is passed) for inbound timezone.
> 
> Do you have an example where this "Z" is passed?

Z is part of the RFC ;).  I don't yet have an example of where we pass it in Koha, but it's valid via the API and so should be catered for.
Comment 39 Martin Renvoize 2020-04-15 07:56:17 UTC
https://github.com/mojolicious/json-validator/blob/master/lib/JSON/Validator/Formats.pm#L35 shows we support passing Z in our validation routines.

https://tools.ietf.org/html/rfc3339#section-2 for reference in the RFC
Comment 40 Marcel de Rooy 2020-04-17 07:26:36 UTC
Part of the rfc3339 regex: ([Tt\s])
I am not sure if lowercase t is part of the syntax. I think it should be capital T or space. Note that \s is more than space. I think we should test for space only here.

        # Default to UTC (when 'Z' is passed) for inbound timezone.
        $tz = DateTime::TimeZone->new( name => 'UTC' );

Yes, Z stands for UTC. But this line comes out of the blue. We are just building the regex here.
Further down, we are testing for the offset and at that code location we should check for the Z too and set UTC timezone.

+    # Convert to configured timezone (unless we started with a dateonly string or had to drop to floating time)
+    $dt->set_time_zone($server_tz) unless ( $date_only || $floating );

This line is confusing too. Although I am not sure if we have calls in Koha where a timezone is passed already, so it may be theoretical. But aren't we effectively ignoring/overriding the $tz parameter here?
Should this routine have a tz parameter anyhow?
Note also that we did not interfere earlier in the code: 
    return DateTime->now( time_zone => $tz )
In line with what we do at the end, we should not allow this return?
So, actually the big question is here: Shouldnt we always return in server timezone ?

Changing status.
Comment 41 Martin Renvoize 2020-04-23 08:02:51 UTC
The idea of the input tz is that you're passing in the TZ of the string being passed in.. the idea of the tz conversion at the bottom (and in the call to now) is that we always pass back a DateTime object with a consistent config based timezone.  doing this call at the bottom means we will convert internally to the correct instance timezone from the passed timezone.
Comment 42 Martin Renvoize 2020-07-15 12:41:00 UTC
Did my reply make sense Marcel.. or are you still unhappy with this work.
Comment 43 Marcel de Rooy 2020-07-15 12:42:38 UTC
(In reply to Martin Renvoize from comment #42)
> Did my reply make sense Marcel.. or are you still unhappy with this work.

I am afraid that it got lost between lots of other patch mails..
Keeping this one for Friday..
Comment 44 Martin Renvoize 2020-07-15 19:32:37 UTC
No worries... I forgot about it too.. might take a bit to get the brain back up to speed.
Comment 45 Marcel de Rooy 2020-07-17 09:09:20 UTC
Please rebase
Comment 46 Marcel de Rooy 2020-07-17 09:11:43 UTC
(In reply to Martin Renvoize from comment #41)
> The idea of the input tz is that you're passing in the TZ of the string
> being passed in.. the idea of the tz conversion at the bottom (and in the
> call to now) is that we always pass back a DateTime object with a consistent
> config based timezone.  doing this call at the bottom means we will convert
> internally to the correct instance timezone from the passed timezone.

Ok. But I have the impression that many questions raised in comment40 are not answered yet..
Comment 47 Martin Renvoize 2021-01-27 08:56:39 UTC
Created attachment 115851 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff
Comment 48 Martin Renvoize 2021-01-27 08:56:43 UTC
Created attachment 115852 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.
Comment 49 Martin Renvoize 2021-01-27 08:56:46 UTC
Created attachment 115853 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.
Comment 50 Martin Renvoize 2021-01-27 08:56:50 UTC
Created attachment 115854 [details] [review]
Bug 24850: Don't fallback when date_format is passed
Comment 51 Martin Renvoize 2021-01-27 08:56:54 UTC
Created attachment 115855 [details] [review]
Bug 24850: Always return 'instance' local time

This patch adds handling to ensure we always convert a passed in time to
the instance configured timezone..
Comment 52 Martin Renvoize 2021-01-27 08:56:57 UTC
Created attachment 115856 [details] [review]
Bug 24850: Continue to handle default time and daylight saving

This patch prevents the call to set_time_zone if we are handling a
dateonly datetime string or a DST time
Comment 53 Martin Renvoize 2021-01-27 09:04:10 UTC
This is blocking bug 24608 and I don't know how to move it on.. I've answered the questions and asked for feedback if the answer isn't understood.. so much time has passed that without going back to the begging of all these issues I can't really give more detail...

But 24608 highlights the issues we have by not properly handling offsets and this bug resolves all those issues whilst maintaining existing compatibility.
Comment 54 Martin Renvoize 2021-01-27 09:19:28 UTC
Created attachment 115857 [details] [review]
Bug 24850: (QA follow-up) Add more comments

To try and clarify the logic here I've added an addition comment to the
code.
Comment 55 David Cook 2021-01-27 23:56:36 UTC
I'm still curious why we don't use DateTime::Format::Strptime (from the author of DateTime).

That said, it doesn't allow for optional parts of patterns, so you have to do a few tests from more specific to less specific.
Comment 56 David Cook 2021-01-27 23:57:07 UTC
Comment on attachment 115852 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

Review of attachment 115852 [details] [review]:
-----------------------------------------------------------------

::: Koha/DateUtils.pm
@@ +117,4 @@
>              (?<minute>\d{2})
>              :
>              (?<second>\d{2})
> +            (\.\d{1,3})?(([Zz])|((?<offset>[\+|\-])(?<hours>[01][0-9]|2[0-3]):(?<minutes>[0-5][0-9])))

I don't think lowercase "z" is valid here?
Comment 57 Martin Renvoize 2021-01-28 09:00:36 UTC
From memory, I think you are right that the Z should always be capitalised.. but I think I read somewhere that most libraries are lenient with the case so I adopted that approach..  Happy to switch it out of course.
Comment 58 Martin Renvoize 2021-01-28 09:01:18 UTC
(In reply to David Cook from comment #55)
> I'm still curious why we don't use DateTime::Format::Strptime (from the
> author of DateTime).
> 
> That said, it doesn't allow for optional parts of patterns, so you have to
> do a few tests from more specific to less specific.

Mmmm, I've often wondered that... history.. Though I note that lib seems to have had a bunch of fixes recently.. so it wasn't perfect.. might be better now.
Comment 59 David Cook 2021-01-28 22:09:05 UTC
(In reply to Martin Renvoize from comment #58)
> Mmmm, I've often wondered that... history.. Though I note that lib seems to
> have had a bunch of fixes recently.. so it wasn't perfect.. might be better
> now.

I think in the past we were using a different library. It was DateTime::Format::DateParse which is super old and unmaintained. DateTime::Format::Strptime, on the other hand, is well maintained. I have an application that deals a lot with time and I love DateTime::Format::Strptime for it heh.
Comment 60 David Cook 2021-02-04 03:36:55 UTC
Comment on attachment 115855 [details] [review]
Bug 24850: Always return 'instance' local time

Review of attachment 115855 [details] [review]:
-----------------------------------------------------------------

::: Koha/DateUtils.pm
@@ +54,5 @@
>      return if $date_string and $date_string =~ m|^0000-0|;
>  
>      my $do_fallback = defined($date_format) ? 0 : 1;
> +    my $server_tz = C4::Context->tz;
> +    $tz = C4::Context->tz unless $tz;

This might read easier if it said the following:

$tz = $server_tz unless $tz;
Comment 61 David Cook 2021-02-04 03:37:44 UTC
Comment on attachment 115852 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

Review of attachment 115852 [details] [review]:
-----------------------------------------------------------------

::: Koha/DateUtils.pm
@@ +160,5 @@
>              second => $+{second},
>          );
>          $ampm = $+{ampm};
> +        if ( $+{offset} ) {
> +            $tz = DateTime::TimeZone->new( name => $+{offset} . $+{hours} . $+{minutes} );

You might want to call them offset_hours and offset_minutes, because they're so close to "hour" and "minute" that I had to do a double-take to make sure they were right.
Comment 62 David Cook 2021-02-04 03:47:54 UTC
I actually don't have the brainpower to QA this one today, but since it's marked as "Needs Signoff", I can at least do that. 

My test plan:
0) Use koha-testing-docker
1) prove t/DateUtils.t
2) Noted "All tests successful."
Comment 63 David Cook 2021-02-04 03:50:28 UTC
Honestly I really dislike dt_from_string. At some point, it should probably have its internals refactored into a number of more specific private sub-functions. But that change is outside the scope of this change.
Comment 64 David Cook 2021-02-04 03:51:21 UTC
Created attachment 116299 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 65 David Cook 2021-02-04 03:51:25 UTC
Created attachment 116300 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 66 David Cook 2021-02-04 03:51:29 UTC
Created attachment 116301 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 67 David Cook 2021-02-04 03:51:33 UTC
Created attachment 116302 [details] [review]
Bug 24850: Don't fallback when date_format is passed

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 68 David Cook 2021-02-04 03:51:37 UTC
Created attachment 116303 [details] [review]
Bug 24850: Always return 'instance' local time

This patch adds handling to ensure we always convert a passed in time to
the instance configured timezone..

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 69 David Cook 2021-02-04 03:51:41 UTC
Created attachment 116304 [details] [review]
Bug 24850: Continue to handle default time and daylight saving

This patch prevents the call to set_time_zone if we are handling a
dateonly datetime string or a DST time

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 70 David Cook 2021-02-04 03:51:45 UTC
Created attachment 116305 [details] [review]
Bug 24850: (QA follow-up) Add more comments

To try and clarify the logic here I've added an addition comment to the
code.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 71 David Cook 2021-02-04 03:52:26 UTC
Comment on attachment 115854 [details] [review]
Bug 24850: Don't fallback when date_format is passed

Review of attachment 115854 [details] [review]:
-----------------------------------------------------------------

::: Koha/DateUtils.pm
@@ +146,4 @@
>                  )?
>              )?
>      |xms;
> +    $regex .= $time_re unless ( $date_format eq 'rfc3339' );

Should this be when $date_format eq 'rfc3339' or any time $date_format is passed?
Comment 72 David Cook 2021-02-04 03:55:30 UTC
I think something I find challenging about this patch set is that it makes a number of changes beyond including support for the timezone offset.

I think it might be easier to get this through if it were broken up into different bugzilla issues. For instance:

1. Correct tests for offset handling in RFC3339 dates
2. Don't fallback when date_format is passed
3. Correct offset handling in dt_from_string
4. Always return 'instance' local time

Honestly, the number of different changes just makes me less confident that I'm thoroughly testing everything and that makes me uncomfortable QAing this one.
Comment 73 Marcel de Rooy 2021-02-05 07:25:37 UTC
QA: Looking here
Comment 74 Marcel de Rooy 2021-02-05 07:33:53 UTC Comment hidden (obsolete)
Comment 75 Marcel de Rooy 2021-02-05 07:35:17 UTC Comment hidden (obsolete)
Comment 76 Marcel de Rooy 2021-02-05 08:00:24 UTC
Created attachment 116355 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 77 Marcel de Rooy 2021-02-05 08:00:27 UTC
Created attachment 116356 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 78 Marcel de Rooy 2021-02-05 08:00:31 UTC
Created attachment 116357 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 79 Marcel de Rooy 2021-02-05 08:00:35 UTC
Created attachment 116358 [details] [review]
Bug 24850: Don't fallback when date_format is passed

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 80 Marcel de Rooy 2021-02-05 08:00:38 UTC
Created attachment 116359 [details] [review]
Bug 24850: Always return 'instance' local time

This patch adds handling to ensure we always convert a passed in time to
the instance configured timezone..

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 81 Marcel de Rooy 2021-02-05 08:00:42 UTC
Created attachment 116360 [details] [review]
Bug 24850: Continue to handle default time and daylight saving

This patch prevents the call to set_time_zone if we are handling a
dateonly datetime string or a DST time

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 82 Marcel de Rooy 2021-02-05 08:00:46 UTC
Created attachment 116361 [details] [review]
Bug 24850: (QA follow-up) Add more comments

To try and clarify the logic here I've added an addition comment to the
code.

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 83 Jonathan Druart 2021-02-08 14:42:33 UTC
(In reply to Marcel de Rooy from comment #79)
> Created attachment 116358 [details] [review] [review]
> Bug 24850: Don't fallback when date_format is passed

I was trying to understand where this $do_fallback was tested. I removed it to the following line:

+    } elsif ( $do_fallback && $date_string =~ $fallback_re ) {

but the test is not failing explicitly:
  t/DateUtils.t .. 1/80 # Looks like you planned 80 tests but ran 79.


Can you highlight this use case in the tests please?
Comment 84 Jonathan Druart 2021-02-08 14:47:14 UTC
Created attachment 116511 [details] [review]
Bug 24850: (follow-up) Add tests for invalid RFC3339 dates

Make this failure explicit, otherwise we get a "bad plan" failure.
Comment 85 Jonathan Druart 2021-02-08 14:54:38 UTC
I think this line is not tested:

229     $dt->set_time_zone($server_tz) unless ( $date_only || $floating );

And cannot we remove $floating and compare something like $dt->time_zone eq 'floating'?
Comment 86 Martin Renvoize 2021-02-08 18:50:03 UTC
Sorry chaps, I'll swing back around to this tomorrow.. been a bit bogged down the past few days
Comment 87 Martin Renvoize 2021-02-10 09:13:12 UTC
(In reply to David Cook from comment #72)
> I think something I find challenging about this patch set is that it makes a
> number of changes beyond including support for the timezone offset.
> 
> I think it might be easier to get this through if it were broken up into
> different bugzilla issues. For instance:
> 
> 1. Correct tests for offset handling in RFC3339 dates
> 2. Don't fallback when date_format is passed
> 3. Correct offset handling in dt_from_string
> 4. Always return 'instance' local time
> 
> Honestly, the number of different changes just makes me less confident that
> I'm thoroughly testing everything and that makes me uncomfortable QAing this
> one.

Whilst I agree.. I think that's how I started but found that half the tests failed if I didn't follow through in one step.

So much time has passed since I first wrote these patches that I must admit I'm struggling to unpick them myself :(.

I'm going to take a look at refactoring the tests to get my head around what each test is actually testing.
Comment 88 Martin Renvoize 2021-10-13 12:38:26 UTC
Created attachment 126182 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 89 Martin Renvoize 2021-10-13 12:38:31 UTC
Created attachment 126183 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 90 Martin Renvoize 2021-10-13 12:38:35 UTC
Created attachment 126184 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 91 Martin Renvoize 2021-10-13 12:38:41 UTC
Created attachment 126185 [details] [review]
Bug 24850: Don't fallback when date_format is passed

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 92 Martin Renvoize 2021-10-13 12:38:46 UTC
Created attachment 126186 [details] [review]
Bug 24850: Always return 'instance' local time

This patch adds handling to ensure we always convert a passed in time to
the instance configured timezone..

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 93 Martin Renvoize 2021-10-13 12:38:52 UTC
Created attachment 126187 [details] [review]
Bug 24850: Continue to handle default time and daylight saving

This patch prevents the call to set_time_zone if we are handling a
dateonly datetime string or a DST time

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 94 Martin Renvoize 2021-10-13 12:38:57 UTC
Created attachment 126188 [details] [review]
Bug 24850: (QA follow-up) Add more comments

To try and clarify the logic here I've added an addition comment to the
code.

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 95 Martin Renvoize 2021-10-13 12:39:03 UTC
Created attachment 126189 [details] [review]
Bug 24850: (follow-up) Add tests for invalid RFC3339 dates

Make this failure explicit, otherwise we get a "bad plan" failure.
Comment 96 Tomás Cohen Arazi 2021-10-17 23:14:50 UTC
What is the status of this bug? I find the change pretty simple and the tests are clear as well. I see sign offs from a bunch of people as well. I tested it too, but not sure what is the real status.
Comment 97 Jonathan Druart 2021-10-18 06:25:21 UTC
(In reply to Tomás Cohen Arazi from comment #96)
> What is the status of this bug? I find the change pretty simple and the
> tests are clear as well. I see sign offs from a bunch of people as well. I
> tested it too, but not sure what is the real status.

See comments 85-87
Comment 98 Martin Renvoize 2021-10-22 12:41:14 UTC
I have nothing to add here..   This bug is a massive blocker for any API bugs that deal with dates.. to me the tests are as clear as I can make them and I don't trust myself to re-write them all.. that's kinda the point of tests.. to prevent regressions.. re-writing them all at this point would be asking for regressions.
Comment 99 Katrin Fischer 2021-10-22 12:47:06 UTC
Ok, so we need some more eyes/typing hands/Perl brains on this one. 

Joubu, could you clarify maybe the points that need attention before inclusion?
Comment 100 Katrin Fischer 2021-10-22 12:50:00 UTC
Deciding on "in Discussion" as we concluded that those brains/hands/eyes need to meet up to discuss the path forward with timezone handling first.
Comment 101 Martin Renvoize 2021-10-22 12:54:12 UTC
API's that deal with dates in any way are fundamentally broken without the patches here.. so I will be failing anything that touches dates in API's until this has been looked at.

I personally don't have the mental capacity to go down this road for another time so am stepping away. Someone else will have to take this one.

Burnt out...
Comment 102 Jonathan Druart 2021-10-22 14:02:49 UTC
Tested on top of bug 24609.

This script:
"""
use Koha::DateUtils qw( dt_from_string output_pref );
use Koha::Patrons;
use C4::Context;
use C4::Circulation qw(AddIssue );
use t::lib::Mocks;
t::lib::Mocks::mock_config('America/Cordoba');
my $dbh = C4::Context->dbh;
my $patron = Koha::Patron->new({userid => 'xxx', categorycode => 'S', branchcode => 'CPL'})->store;
t::lib::Mocks::mock_userenv({patron => $patron});
my $item = Koha::Items->find(1);
my $today = dt_from_string;
say "NOW: " . $today. " : " . $today->time_zone_short_name;
my $issue = AddIssue($patron->unblessed, $item->barcode);
my $date_due = $issue->date_due;
say "OBJECT    :" . $date_due;
say "UI        :" . output_pref({str => $date_due});
say "ID: " . $issue->id;
"""

Creates an checkout and outputs:
NOW: 2021-10-22T10:46:15 : -03
OBJECT    :2021-10-27 23:59:00
UI        :10/27/2021 23:59
ID: 4

GET http://kohadev-intra.mydnsname.org:8081/api/v1/checkouts/4
=> "due_date": "2021-10-27T23:59:00-03:00",

PUT http://kohadev-intra.mydnsname.org:8081/api/v1/checkouts/4
{
    "due_date": "2021-10-27T23:59:00-03:00",
    "issuer_id": null,
    "note_seen": false,
    "patron_id": 179,
    "unseen_renewals": 0
}
GET http://kohadev-intra.mydnsname.org:8081/api/v1/checkouts/4
=> "due_date": "2021-10-27T23:59:00-03:00",

PUT http://kohadev-intra.mydnsname.org:8081/api/v1/checkouts/4
{
    "due_date": "2021-10-27T20:59:00-06:00",
    "issuer_id": null,
    "note_seen": false,
    "patron_id": 179,
    "unseen_renewals": 0
}
GET http://kohadev-intra.mydnsname.org:8081/api/v1/checkouts/4
=> "due_date": "2021-10-27T23:59:00-03:00",

PUT http://kohadev-intra.mydnsname.org:8081/api/v1/checkouts/4
{
    "due_date": "2021-10-27T23:59:00-06:00",
    "issuer_id": null,
    "note_seen": false,
    "patron_id": 179,
    "unseen_renewals": 0
}
GET http://kohadev-intra.mydnsname.org:8081/api/v1/checkouts/4
=> "due_date": "2021-10-28T02:59:00-03:00",
Comment 103 Jonathan Druart 2021-10-22 14:46:48 UTC
I haven't tried what's happening if no TZ is passed. Do we reject it, assuming it's instance tz or UTC?
Comment 104 Jonathan Druart 2021-10-25 08:49:45 UTC
(In reply to Jonathan Druart from comment #103)
> I haven't tried what's happening if no TZ is passed. Do we reject it,
> assuming it's instance tz or UTC?

It's mandatory. However this is weird:

    "due_date": "2021-10-27 23:59",
=> "Does not match date-time"

    "due_date": "2021-10-27 23:59:00",
=> "Missing property."
Comment 105 Martin Renvoize 2021-10-26 13:42:56 UTC
(In reply to Jonathan Druart from comment #104)
> (In reply to Jonathan Druart from comment #103)
> > I haven't tried what's happening if no TZ is passed. Do we reject it,
> > assuming it's instance tz or UTC?
> 
> It's mandatory. However this is weird:
> 
>     "due_date": "2021-10-27 23:59",
> => "Does not match date-time"
> 
>     "due_date": "2021-10-27 23:59:00",
> => "Missing property."

Added as bug 29322.. I think it's tied up inside JSON::Validator.. I'll ask Tomas to take a look.
Comment 106 Jonathan Druart 2021-11-02 15:52:46 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 107 Jonathan Druart 2021-11-02 16:41:32 UTC
There is a test failing because of this:

t/db_dependent/Koha/Object.t .. 9/21         # No tests run!

    #   Failed test 'No tests run for subtest "date and date-time handling tests"'
    #   at t/db_dependent/Koha/Object.t line 649.
    # Looks like you planned 2 tests but ran 1.
    # Looks like you failed 1 test of 1 run.

#   Failed test 'attributes_from_api() tests'
#   at t/db_dependent/Koha/Object.t line 669.
[A bad parameter was given]# Looks like your test exited with 255 just after 14.
Comment 108 Jonathan Druart 2021-11-02 16:44:09 UTC
And many more actually, see the last builds
Comment 109 Jonathan Druart 2021-11-02 16:51:09 UTC
(In reply to Jonathan Druart from comment #108)
> And many more actually, see the last builds

So for Object.t it's failing on date_or_birth because of:
The given date (2019-12-27) does not match the date format (rfc3339) at /kohadevbox/koha/Koha/DateUtils.pm line 193.

We lost the fallback, certainly caused by 'Bug 24850: Don't fallback when date_format is passed'
Comment 110 Jonathan Druart 2021-11-02 16:55:22 UTC
We could assume that 'date' won't be rfc3339, but I am not sure it's correct.
The following patches seem to fix the tests.

diff --git a/Koha/Object.pm b/Koha/Object.pm
index 5d0f3f720ff..ec1316c45d7 100644
--- a/Koha/Object.pm
+++ b/Koha/Object.pm
@@ -751,7 +751,7 @@ sub attributes_from_api {
         elsif ( _date_or_datetime_column_type( $columns_info->{$koha_field_name}->{data_type} ) ) {
             try {
                 if ( $columns_info->{$koha_field_name}->{data_type} eq 'date' ) {
-                    $value = $dtf->format_date(dt_from_string($value, 'rfc3339'))
+                    $value = $dtf->format_date(dt_from_string($value))
                         if defined $value;
                 }
                 else {
Comment 111 Tomás Cohen Arazi 2021-11-02 21:07:51 UTC
This commit broke all the things:

398f7b45f074b023b03e5075821f0abb08cda66c
Comment 112 Tomás Cohen Arazi 2021-11-02 21:23:18 UTC
That's what we're getting.

The given date (2021-11-02) does not match the date format (rfc3339)
Comment 113 Jonathan Druart 2021-11-03 09:09:26 UTC
Created attachment 127231 [details] [review]
Bug 24850: Date only are not RFC3339

We removed the fallback on 'Bug 24850: Don't fallback when date_format
is passed', and the dates passed for date only (YYYY-MM-DD) are not
RFC3339 formatted but ISO8601.
We must assume that all dates will be YYYY-MM-DD, without fallback.
Comment 114 Jonathan Druart 2021-11-03 09:10:22 UTC
Created attachment 127232 [details] [review]
Bug 24850: Date only are not RFC3339

We removed the fallback on 'Bug 24850: Don't fallback when date_format
is passed', and the dates passed for date only (YYYY-MM-DD) are not
RFC3339 formatted but ISO8601.
We must assume that all dates will be YYYY-MM-DD, without fallback.
Comment 115 Tomás Cohen Arazi 2021-11-03 09:18:57 UTC
Date-only is part of RFC 3339. It is the full-date spec which matches ISO 8601 [1]

Maybe we need a dateonly param so things are clear?

[1] https://xml2rfc.tools.ietf.org/public/rfc/html/rfc3339.html#anchor14
Comment 116 Martin Renvoize 2021-11-03 09:33:30 UTC
I actually read it the other way around.. RFC3339 is full date-time specific and is a subspec of ISO8609 which is looser and allows for date only.
Comment 117 Martin Renvoize 2021-11-03 09:34:46 UTC
https://stackoverflow.com/questions/522251/whats-the-difference-between-iso-8601-and-rfc-3339-date-formats

Highlighting the bit that raised my thoughts: `Most notably RFC 3339 specifies a complete representation of date and time (only fractional seconds are optional)`
Comment 118 Martin Renvoize 2021-11-03 09:35:50 UTC
But yes.. I did wonder if we should actually have a 'date only' parameter we can pass so our iso fallback can explicitly ban time parts for this case.
Comment 119 Martin Renvoize 2021-11-03 09:37:50 UTC
But, for here.. I think passing 'iso' as the format explicitly for date only where Jonathans follow-up does seems like the most pragmatic solution in the time.. Adding a 'date-only' parameter should be in a desperate bug in my opinion.
Comment 120 Jonathan Druart 2021-11-03 09:45:55 UTC
There is still one test failing:

    #   Failed test '404 Not Found'
    #   at t/db_dependent/api/v1/holds.t line 220.
    #          got: '400'
    #     expected: '404'

    #   Failed test 'has value for JSON Pointer "/error"'
    #   at t/db_dependent/api/v1/holds.t line 220.

I don't understand it and it's a mess to debug (We definitely need to improve our error handling when testing the REST API).
Comment 121 Jonathan Druart 2021-11-03 09:46:15 UTC
Created attachment 127235 [details] [review]
Bug 24850: Fix api/v1/holds.t
Comment 122 Jonathan Druart 2021-11-03 09:46:50 UTC
Read those 2 comments the other way around, there is still one test failing :)
Comment 123 Tomás Cohen Arazi 2021-11-03 09:48:24 UTC
(In reply to Martin Renvoize from comment #119)
> But, for here.. I think passing 'iso' as the format explicitly for date only
> where Jonathans follow-up does seems like the most pragmatic solution in the
> time.. Adding a 'date-only' parameter should be in a desperate bug in my
> opinion.

I agree, what I meant to say is that passing an ISO full-date formatted date is correct in the Swagger 2.0 spec, and we should better have explicit params. My attempt yesterday night was to add something explicit:

$value = $dtf->format_datetime(dt_from_string($value, 'iso'))

but either works, as long as we document this conversation
Comment 124 Tomás Cohen Arazi 2021-11-03 09:55:39 UTC
Created attachment 127236 [details] [review]
Bug 24850: suspended_until is a date-time

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 125 Jonathan Druart 2021-11-03 10:01:35 UTC
 77     "suspended_until": {
 78       "type": ["string", "null"],
 79       "format": "date-time",
 80       "description": "Date until which the hold has been suspended"
 81     },

But the code is converting everything to date-only...

What do we want here?
Comment 126 Tomás Cohen Arazi 2021-11-03 10:14:09 UTC
(In reply to Jonathan Druart from comment #125)
>  77     "suspended_until": {
>  78       "type": ["string", "null"],
>  79       "format": "date-time",
>  80       "description": "Date until which the hold has been suspended"
>  81     },
> 
> But the code is converting everything to date-only...
> 
> What do we want here?

The spec was written taking the DB structure into account. If you think the spec is wrong and this field should be date-only, let's do it on a separate bug?
Comment 127 Jonathan Druart 2021-11-03 10:28:41 UTC
/suspension is expecting a date
other routes are expecting a datetime

That seems inconsistent and wrong. IMO we should open a separate bug report and base the test fix on top of it.
Comment 128 Jonathan Druart 2021-11-03 11:00:27 UTC
Created attachment 127239 [details] [review]
Bug 24850: Date only are not RFC3339

We removed the fallback on 'Bug 24850: Don't fallback when date_format
is passed', and the dates passed for date only (YYYY-MM-DD) are not
RFC3339 formatted but ISO8601.
We must assume that all dates will be YYYY-MM-DD, without fallback.
Comment 129 Jonathan Druart 2021-11-03 11:16:44 UTC
Created attachment 127240 [details] [review]
Bug 24850: Correctly handle suspended_until for PUT

if we are passing suspended_until to the body, it should be a rfc3339 (what the spec is expecting)
but $hold->suspend_until will be iso
Comment 130 Martin Renvoize 2021-11-03 11:38:57 UTC
Created attachment 127241 [details] [review]
Bug 24850: Fix api/v1/holds.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 131 Martin Renvoize 2021-11-03 11:39:02 UTC
Created attachment 127242 [details] [review]
Bug 24850: suspended_until is a date-time

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 132 Martin Renvoize 2021-11-03 11:39:07 UTC
Created attachment 127243 [details] [review]
Bug 24850: Date only are not RFC3339

We removed the fallback on 'Bug 24850: Don't fallback when date_format
is passed', and the dates passed for date only (YYYY-MM-DD) are not
RFC3339 formatted but ISO8601.
We must assume that all dates will be YYYY-MM-DD, without fallback.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 133 Martin Renvoize 2021-11-03 11:39:11 UTC
Created attachment 127244 [details] [review]
Bug 24850: Correctly handle suspended_until for PUT

if we are passing suspended_until to the body, it should be a rfc3339 (what the spec is expecting)
but $hold->suspend_until will be iso

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 134 Martin Renvoize 2021-11-03 11:41:36 UTC
OK.. I've tested the follow-ups and agree that, although not perfect in the wider scheme, they are what we should push for now.. as such I've signed off.

I have added two new bugs depending on this one however, to continue the work to clarify and improve our date handling situation.
Comment 135 Jonathan Druart 2021-11-03 14:36:26 UTC
Last patches pushed to master.
Comment 136 Kyle M Hall 2021-11-05 12:52:17 UTC
Unit tests fail in 21.05.x, please rebase and ping me if this is needed for 21.05.x