Bug 20178

Summary: Fix output_pref with dateformat=iso
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: Test SuiteAssignee: Chris Cormack <chris>
Status: NEW --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: jonathan.druart, kyle, mtompset
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20199
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20008
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 20178: [sql_modes] Fix output_pref with dateformat=iso
Bug 20178: Fix test - iso formatted dates should not use timeformat
Bug 20178: [sql_modes] Fix output_pref with dateformat=iso
Bug 20178: Fix test - iso formatted dates should not use timeformat

Description Julian Maurice 2018-02-12 13:49:28 UTC
This bug is a continuation of bug 20144
Comment 1 Julian Maurice 2018-02-12 13:50:24 UTC
Created attachment 71469 [details] [review]
Bug 20178: [sql_modes] Fix output_pref with dateformat=iso

Fix for:
Incorrect date value: '2018-02-05 06:30 PM' for column 'entrydate'

TODO - check other occurrences, put to a separate bug report
Then `git grep output_pref|grep iso` and make sure no other calls are
impacted.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 2 Julian Maurice 2018-02-12 13:52:09 UTC
This patch makes t/DateUtils.t fail

#   Failed test 'iso output 12hr'
#   at t/DateUtils.t line 47.
#          got: '2011-06-16 12:00'
#     expected: '2011-06-16 12:00 PM'
# Looks like you failed 1 test of 67.
Comment 3 Jonathan Druart 2018-02-12 19:06:45 UTC
Created attachment 71498 [details] [review]
Bug 20178: Fix test - iso formatted dates should not use timeformat
Comment 4 Jonathan Druart 2018-02-12 19:07:45 UTC
*** Bug 20008 has been marked as a duplicate of this bug. ***
Comment 5 Mark Tompsett 2018-02-14 03:46:31 UTC
Created attachment 71593 [details] [review]
Bug 20178: [sql_modes] Fix output_pref with dateformat=iso

Fix for:
Incorrect date value: '2018-02-05 06:30 PM' for column 'entrydate'

TODO - check other occurrences, put to a separate bug report
Then `git grep output_pref|grep iso` and make sure no other calls are
impacted.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 6 Mark Tompsett 2018-02-14 03:46:34 UTC
Created attachment 71594 [details] [review]
Bug 20178: Fix test - iso formatted dates should not use timeformat

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 7 Julian Maurice 2018-02-14 08:15:03 UTC
What if I want to display dates in ISO format but time in 12 hours format ? It is actually possible in master, but this patch prevents this.
Comment 8 Mark Tompsett 2018-02-14 13:51:27 UTC
(In reply to Julian Maurice from comment #7)
> What if I want to display dates in ISO format but time in 12 hours format ?
> It is actually possible in master, but this patch prevents this.

ISO format includes times, and they must be 24 hour. This is why I took a little longer signing off, because I had to confirm that 12 hour times were not valid in ISO formatting.

You could custom tweak your system preference to something that triggers the fallback else case. :)
Comment 9 Jonathan Druart 2018-02-14 14:03:21 UTC
Yes, what Mark said, dateformat='iso' means, actually, a 'sql' format.

This is an historical issue (introduced by me when I improved Koha::DateUtils), we should distinguish these 2 formats but we do not have this need for so far.

But in both case the date should be formatted with a date in 24h format.
Comment 10 Julian Maurice 2018-02-14 14:23:25 UTC
It looks like a misuse of output_pref. Shouldn't the result of this subroutine be used only for display ?

Anyway there is a lack of documentation here. It should be documented at least in POD and in description of both sysprefs IMO.
Comment 11 Jonathan Druart 2018-02-14 16:57:29 UTC
I forgot this value existed in the syspref, so I guess this is not the right fix.
I am wondering if this value is really in used, I am expecting several side-effects using it.

Maybe we should fix the test by mocking the pref instead.
Comment 12 Jonathan Druart 2018-02-14 17:18:50 UTC
This need to be fixed globally or at least check all the occurrences to estimate the issue.
Comment 13 Jonathan Druart 2018-02-14 17:21:49 UTC
I have opened bug 20199 for the Letters.t failure.
Comment 14 Katrin Fischer 2023-10-01 21:08:11 UTC
I am not sure how to test this - can someone tell if it's still valid?