|
Description
Martin Renvoize (ashimema)
2020-03-11 15:23:15 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 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. 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. 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 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. 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. Created attachment 100595 [details] [review] Bug 24850: Don't fallback when rfc3339 format is passed 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. 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. 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 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. 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. Created attachment 100681 [details] [review] Bug 24850: Don't fallback when date_format is passed (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. 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.. 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 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. 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. Created attachment 100904 [details] [review] Bug 24850: Don't fallback when date_format is passed 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.. Martin, t/DateUtils.t fails always. An special setup is needed? (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. 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 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. 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. Created attachment 101806 [details] [review] Bug 24850: Don't fallback when date_format is passed 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.. 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 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> 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> 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> 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> 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> Created attachment 101842 [details] [review] Bug 24850: Don't fallback when date_format is passed Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> 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> 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> + # Default to UTC (when 'Z' is passed) for inbound timezone. Do you have an example where this "Z" is passed? (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. 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 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.
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. Did my reply make sense Marcel.. or are you still unhappy with this work. (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.. No worries... I forgot about it too.. might take a bit to get the brain back up to speed. Please rebase (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.. 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 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. 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. Created attachment 115854 [details] [review] Bug 24850: Don't fallback when date_format is passed 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.. 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 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. 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. 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 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? 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. (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. (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 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 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. 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." 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. 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> 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> 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> Created attachment 116302 [details] [review] Bug 24850: Don't fallback when date_format is passed Signed-off-by: David Cook <dcook@prosentient.com.au> 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> 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> 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 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? 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. QA: Looking here From the RFC:
time-second = 2DIGIT ; 00-58, 00-59, 00-60 based on leap second
; rules
And the code:
+ (\.\d{1,3})?(([Zz])|((?<offset>[\+|\-])(?<hours>[01][0-9]|2[0-3]):(?<minutes>[0-5][0-9])))
No blocker, just noting
(In reply to Marcel de Rooy from comment #74) > From the RFC: > time-second = 2DIGIT ; 00-58, 00-59, 00-60 based on leap second > ; rules > > And the code: > + > (\.\d{1,3})?(([Zz])|((?<offset>[\+|\-])(?<hours>[01][0-9]|2[0-3]): > (?<minutes>[0-5][0-9]))) > > No blocker, just noting No, thats fine. Looking at seconds.. 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> 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> 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> 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> 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> 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> 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> (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? 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. 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'? Sorry chaps, I'll swing back around to this tomorrow.. been a bit bogged down the past few days (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. 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> 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> 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> 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> 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> 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> 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> 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. 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. (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 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. 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? 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. 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... 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", I haven't tried what's happening if no TZ is passed. Do we reject it, assuming it's instance tz or 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." (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. Pushed to master for 21.11, thanks to everybody involved! 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.
And many more actually, see the last builds (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' 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 {
This commit broke all the things: 398f7b45f074b023b03e5075821f0abb08cda66c That's what we're getting. The given date (2021-11-02) does not match the date format (rfc3339) 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. 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. 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 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. 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)` 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. 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. 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).
Created attachment 127235 [details] [review] Bug 24850: Fix api/v1/holds.t Read those 2 comments the other way around, there is still one test failing :) (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 Created attachment 127236 [details] [review] Bug 24850: suspended_until is a date-time Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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?
(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? /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. 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. 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 Created attachment 127241 [details] [review] Bug 24850: Fix api/v1/holds.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> 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> 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> 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. Last patches pushed to master. Unit tests fail in 21.05.x, please rebase and ping me if this is needed for 21.05.x |