Bug 20178 - Fix output_pref with dateformat=iso
Summary: Fix output_pref with dateformat=iso
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Chris Cormack
QA Contact: Julian Maurice
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-12 13:49 UTC by Julian Maurice
Modified: 2018-02-28 17:37 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20178: [sql_modes] Fix output_pref with dateformat=iso (1.04 KB, patch)
2018-02-12 13:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20178: Fix test - iso formatted dates should not use timeformat (1.02 KB, patch)
2018-02-12 19:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20178: [sql_modes] Fix output_pref with dateformat=iso (1.08 KB, patch)
2018-02-14 03:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20178: Fix test - iso formatted dates should not use timeformat (1.06 KB, patch)
2018-02-14 03:46 UTC, M. Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 M. 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 M. 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 M. 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.