Bug 29718

Summary: DateTime - our 'iso' is not ISO 8601
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: REOPENED --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: dcook, kyle, m.de.rooy, martin.renvoize, mtj, tomascohen, victor
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29711
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23079
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00
Bug Depends on:    
Bug Blocks: 30030, 29403    
Attachments: Bug 29718: Support ISO8601
Bug 29718: Support ISO8601
Bug 29718: (QA follow-up) Make 'T' check case insensative
Bug 29718: (QA follow-up) Add optional offset handling
Bug 29718: Unit tests
Bug 29718: Mock the timezone
Bug 29718: Support ISO8601
Bug 29718: (QA follow-up) Make 'T' check case insensative
Bug 29718: (QA follow-up) Add optional offset handling
Bug 29718: Unit tests
Bug 29718: Mock the timezone
Bug 29718: (QA follow-up) Remove duplicate test
Bug 29718: Support ISO8601
Bug 29718: (QA follow-up) Make 'T' check case insensative
Bug 29718: (QA follow-up) Add optional offset handling
Bug 29718: Unit tests
Bug 29718: Mock the timezone
Bug 29718: (QA follow-up) Remove duplicate test
Bug 29718: (QA follow-up) ISO 8601 allows +02 and +0200

Description Jonathan Druart 2021-12-16 16:00:38 UTC
In some scripts we are doing unnecessary conversion from string to DT and vice-versa, sometimes several times.

For instance, see bug 29711. We have epoch => DT => string => sending to template in 'iso' YYYY-MM-DD HH:MM => DT => string.

This is because the stringified version of DT (the real ISO 8601 YYYY-MM-DDTHH:MM:SS) is not supported by our dt_from_string method.

There are (at least) two options:
* Use DateTime::Format::Strptime

In dt_from_string:
    my $formatter = DateTime::Format::Strptime->new(pattern => '%Y-%m-%d %H:%M:%S');
    return DateTime->now(
        time_zone => $tz,
        formatter => $formatter,
    ) unless $date_string;

Then later
    $dt_params{formatter} = $formatter;

But that's an overhead for all dt_from_string calls.

* add "T?" to $time_re, easy and efficient.
Comment 1 Jonathan Druart 2021-12-16 16:02:43 UTC
Created attachment 128631 [details] [review]
Bug 29718: Support ISO8601
Comment 2 Jonathan Druart 2021-12-16 16:03:34 UTC
Note that this patch fixes bug 29711. We would also remove all the extra output_pref calls from pl scripts.
Comment 3 Jonathan Druart 2021-12-16 16:04:49 UTC
Martin, Tomas, what do you think?
I knew we wanted to be stricter, but here I have the feeling that this patch is correct.
Comment 4 Martin Renvoize 2021-12-17 11:45:11 UTC
Right.. I agree with your patch.. mostly.

In the RFC3339 regex we actually do '([Tt\s])'.. Whilst I think ISO was slightly more strict.. i.e 'T' now and '\s' before (So I agree to allowing for both).. should we also support lower case 't' ?
Comment 5 Martin Renvoize 2021-12-17 11:50:28 UTC
Also.. being slightly picky.. I'd update the comment lines that explain the regex's..

# iso or sql format are yyyy-dd-mm[ hh:mm:ss]"

and

# Add the faculative time part [hh:mm[:ss]]
Comment 6 Martin Renvoize 2021-12-17 11:55:09 UTC
Also, interestingly.. I note that for ISO I don't believe 'am/pm' is actually allowed.. all times should be 24 hour.  That's an oversight I hadn't spotted before..
Comment 7 Martin Renvoize 2021-12-17 12:15:04 UTC
Also.. to properly adhere to ISO we should also optionally support timezone (with it defaulting to "local" when one isn't passed).
Comment 8 Martin Renvoize 2021-12-17 12:21:22 UTC
Created attachment 128653 [details] [review]
Bug 29718: Support ISO8601

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2021-12-17 12:21:26 UTC
Created attachment 128654 [details] [review]
Bug 29718: (QA follow-up) Make 'T' check case insensative

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2021-12-17 12:38:58 UTC
Created attachment 128658 [details] [review]
Bug 29718: (QA follow-up) Add optional offset handling

ISO8601 optionally supports timezone offsets. If a timezone is ommited,
we should assume 'local' time (Which in our case is instance
configuration time), if we are passed 'Z' or 'z' we should set timezone
to 'UTC', otherwise we should set the timezone to the passed offset.

Whilst ISO8601 does NOT support AM/PM, I left that intact as it has
historical relevance in Koha and we share the ISO parsing with SQL type
parsing amongth other things.
Comment 11 Martin Renvoize 2021-12-17 12:44:50 UTC
Clearly, we still need to add some additional unit tests to prove the new support.

Whilst testing I took the following set of strings to test the regex's against:

2020-12-09T16:09:53+00:00
2020-12-09 16:09:53+00:00
2020-12-09T 16:09:53+00:00
2020-12-09t16:09:53+00:00
2020-12-0916:09:53+00:00
2020-12-09  16:09:53+00:00
2020-12-09 16:09:53Z
2020-12-09 04:09:53 pm
2020-12-09 04:09:53 am
2020-12-09 04:09:53

2020-12-09 04:09:53am+00:00 # Fails as it should
2020-12-09 04:09:53+00:00am # Fails as it should
Comment 12 Martin Renvoize 2021-12-21 14:17:32 UTC
Created attachment 128840 [details] [review]
Bug 29718: Unit tests
Comment 13 Jonathan Druart 2022-01-06 14:21:22 UTC
Tests are failing for me

t/DateUtils.t .. 20/79 
    #   Failed test 'dt_from_string with no offset assumes "local"'
    #   at t/DateUtils.t line 160.
    #          got: '1325462399'
    #     expected: '1325455199'

    #   Failed test 'dt_from_string with UTC prescribed as Z'    #   at t/DateUtils.t line 164.
    #          got: '1325462399'
    #     expected: '1325455199'
    # Looks like you failed 2 tests of 6.
Comment 14 Martin Renvoize 2022-01-10 14:41:47 UTC
Interesting.. on k-t-d I can't replicate your failures..?
Comment 15 Martin Renvoize 2022-01-10 16:04:09 UTC
humm.. my local branch misses the unit tests.. wonder why..

I can replicate now I've grabbed the patches here.
Comment 16 Jonathan Druart 2022-01-13 05:48:43 UTC
(In reply to Martin Renvoize from comment #15)
> humm.. my local branch misses the unit tests.. wonder why..
> 
> I can replicate now I've grabbed the patches here.

I am still getting the same failure, patches from bug 29403 and bug 29718 applied.
Comment 17 Jonathan Druart 2022-01-13 05:57:19 UTC
Shouldn't we mock the timezone here?
Comment 18 Jonathan Druart 2022-01-13 06:00:18 UTC
Created attachment 129372 [details] [review]
Bug 29718: Mock the timezone
Comment 19 Martin Renvoize 2022-01-13 08:36:17 UTC
You are indeed right, we needed to mock the timezone.. what I was still trying to wrap my head around was a) what timezone we should pick and b) if it should affect all the existing tests in any way and what the actual times should end up as for the tests..

Looks like you've given that some thought :).. I'll take a look and confirm now.
Comment 20 Martin Renvoize 2022-01-13 09:10:04 UTC
Created attachment 129385 [details] [review]
Bug 29718: Support ISO8601

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2022-01-13 09:10:08 UTC
Created attachment 129386 [details] [review]
Bug 29718: (QA follow-up) Make 'T' check case insensative

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2022-01-13 09:10:12 UTC
Created attachment 129387 [details] [review]
Bug 29718: (QA follow-up) Add optional offset handling

ISO8601 optionally supports timezone offsets. If a timezone is ommited,
we should assume 'local' time (Which in our case is instance
configuration time), if we are passed 'Z' or 'z' we should set timezone
to 'UTC', otherwise we should set the timezone to the passed offset.

Whilst ISO8601 does NOT support AM/PM, I left that intact as it has
historical relevance in Koha and we share the ISO parsing with SQL type
parsing amongth other things.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2022-01-13 09:10:15 UTC
Created attachment 129388 [details] [review]
Bug 29718: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2022-01-13 09:10:19 UTC
Created attachment 129389 [details] [review]
Bug 29718: Mock the timezone

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2022-01-13 09:10:23 UTC
Created attachment 129390 [details] [review]
Bug 29718: (QA follow-up) Remove duplicate test

I'd inadvertantly introduced the exact same test twice!  This patch
removes the duplicate and annotates the test to clarify the timezone
handling expectations.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2022-01-13 09:11:57 UTC
I've tested Jonathans follow-up and added another of my own to annotate the tests and remove a duplicate.

Setting to Signed off to get QA attention as I think we're ready for a last technical eye here now.
Comment 27 Marcel de Rooy 2022-01-14 08:11:16 UTC
QA: Looking here
Comment 28 Marcel de Rooy 2022-01-14 08:13:36 UTC
(In reply to Martin Renvoize from comment #5)
> Also.. being slightly picky.. I'd update the comment lines that explain the
> regex's..
> 
> # iso or sql format are yyyy-dd-mm[ hh:mm:ss]"
> 
> and
> 
> # Add the faculative time part [hh:mm[:ss]]

Even more picky. What about spelling? Facultative?
Comment 29 Marcel de Rooy 2022-01-14 08:18:43 UTC
(?<utc>[Zz]$)|((?<offset>[\+|\-])(?<hours>[01][0-9]|2[0-3]):(?<minutes>[0-5][0-9]))

What slightly confuses is that we use the names hours and minutes here while we also use hour and minute. It is not completely clear that the hours and minutes belong to the tz offset. Better name?
Comment 30 Marcel de Rooy 2022-01-14 08:49:12 UTC
The UTC offset is appended to the time in the same way that 'Z' was above, in the form ±[hh]:[mm], ±[hh][mm], or ±[hh].

So we should be able to specify +02, +0200 as well.
Comment 31 Marcel de Rooy 2022-01-14 09:37:03 UTC
Created attachment 129465 [details] [review]
Bug 29718: Support ISO8601

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2022-01-14 09:37:07 UTC
Created attachment 129466 [details] [review]
Bug 29718: (QA follow-up) Make 'T' check case insensative

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2022-01-14 09:37:11 UTC
Created attachment 129467 [details] [review]
Bug 29718: (QA follow-up) Add optional offset handling

ISO8601 optionally supports timezone offsets. If a timezone is ommited,
we should assume 'local' time (Which in our case is instance
configuration time), if we are passed 'Z' or 'z' we should set timezone
to 'UTC', otherwise we should set the timezone to the passed offset.

Whilst ISO8601 does NOT support AM/PM, I left that intact as it has
historical relevance in Koha and we share the ISO parsing with SQL type
parsing amongth other things.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2022-01-14 09:37:14 UTC
Created attachment 129468 [details] [review]
Bug 29718: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2022-01-14 09:37:17 UTC
Created attachment 129469 [details] [review]
Bug 29718: Mock the timezone

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2022-01-14 09:37:21 UTC
Created attachment 129470 [details] [review]
Bug 29718: (QA follow-up) Remove duplicate test

I'd inadvertantly introduced the exact same test twice!  This patch
removes the duplicate and annotates the test to clarify the timezone
handling expectations.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2022-01-14 09:37:25 UTC
Created attachment 129471 [details] [review]
Bug 29718: (QA follow-up) ISO 8601 allows +02 and +0200

The UTC offset is appended to the time in the form:
    ±[hh]:[mm], ±[hh][mm], or ±[hh]

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Martin Renvoize 2022-01-14 11:53:54 UTC
Nice call regards +02 vs +02:00 and +0200.. I hadn't spotted that.. and.. it turns out I've been spelling facultative wrong my entire life.. lol.
Comment 39 David Cook 2022-01-17 03:46:04 UTC
(In reply to Martin Renvoize from comment #38)
> it turns out I've been spelling facultative wrong my entire life.. lol.

I had the same realization a few months ago haha. I never say it in English (I just say optional) but in French I would say/spell it as faculative too haha. Do people use the word "facultative" much in the UK?
Comment 40 Fridolin Somers 2022-01-28 21:13:18 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 41 Victor Grousset/tuxayo 2022-07-22 21:01:41 UTC
This causes a failure on Ubuntu 18.04: bug 30030
Comment 42 Victor Grousset/tuxayo 2022-08-04 00:46:07 UTC
To have the CI still be meaningful for Ubuntu 18.04 (until 2023-04-26) for Koha 22.05 and in the future 22.11, should these patch be reverted until then? (assuming it's possible to revert)
Comment 43 Tomás Cohen Arazi 2022-08-04 01:31:54 UTC
(In reply to Victor Grousset/tuxayo from comment #42)
> To have the CI still be meaningful for Ubuntu 18.04 (until 2023-04-26) for
> Koha 22.05 and in the future 22.11, should these patch be reverted until
> then? (assuming it's possible to revert)

We need to backport a newer DateTime library for 18.04.
Comment 44 Mason James 2022-08-04 03:04:48 UTC
(In reply to Tomás Cohen Arazi from comment #43)
> (In reply to Victor Grousset/tuxayo from comment #42)
> > To have the CI still be meaningful for Ubuntu 18.04 (until 2023-04-26) for
> > Koha 22.05 and in the future 22.11, should these patch be reverted until
> > then? (assuming it's possible to revert)
> 
> We need to backport a newer DateTime library for 18.04.

i have had some success with updating just the 'libdatetime-timezone-perl' pkg, for u18

my theory is that a newer olsen database is needed for u18 - its been upgraded from 2018d to 2022a
(ps: i cant explain why u20 works with only a 2019c database??)

http://ftp.iana.org/tz/releases/

i did the update some days ago, which fixed the broken tests for master and stable

https://jenkins.koha-community.org/view/master/job/Koha_Master_U18/1510/
https://jenkins.koha-community.org/view/22.05/job/Koha_22.05_U18/44/


i'll aim to update the prod repo soon
Comment 45 Victor Grousset/tuxayo 2022-08-04 17:55:09 UTC
Congratulation Mason :D

Even if there is still mystery we will be able to close bug 30030 when it hit the prod repo, right?
Comment 46 Mason James 2022-08-05 09:11:59 UTC
> my theory is that a newer olsen database is needed for u18 - its been
> upgraded from 2018d to 2022a
> (ps: i cant explain why u20 works with only a 2019c database??)
> 

aah, the 2019b database has fixes for Brazil
  http://changelogs.ubuntu.com/changelogs/pool/universe/libd/libdatetime-timezone-perl/libdatetime-timezone-perl_2.38-1+2019c/changelog

------------------------------------------------------
libdatetime-timezone-perl (1:2.36-1+2019b) unstable; urgency=medium

  * Import upstream version 2.36.
    This release is based on version 2019b of the Olson database.
    It includes contemporary changes for Brazil and Palestine.  <<<
 ------------------------------------------------------
 
updating u18 to libdatetime-timezone-perl_2.23-1+2022a_all.deb fixes the following errors

'Invalid local time for date in time zone: America/Campo_Grande'
'Invalid local time for date in time zone: America/Cuiaba'
'Invalid local time for date in time zone: America/Sao_Paulo'
Comment 47 Mason James 2022-08-05 09:41:28 UTC
reopened, until confirmed fixed
Comment 48 Mason James 2022-08-05 11:37:03 UTC
(In reply to Victor Grousset/tuxayo from comment #45)
> Congratulation Mason :D
> 
> Even if there is still mystery we will be able to close bug 30030 when it
> hit the prod repo, right?

yes, correct :)