Bug 17001 - filtering overdue report by due date can fail if TimeFormat is 12hr
Summary: filtering overdue report by due date can fail if TimeFormat is 12hr
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-29 20:38 UTC by Galen Charlton
Modified: 2017-12-07 22:16 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17001: fix due date filter on the overdue report (2.01 KB, patch)
2016-07-29 21:07 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 17001: fix due date filter on the overdue report (2.06 KB, patch)
2016-08-06 19:00 UTC, Jason Robb
Details | Diff | Splinter Review
k (2.13 KB, patch)
2016-08-07 11:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 17001: fix due date filter on the overdue report (2.13 KB, patch)
2016-08-07 11:55 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2016-07-29 20:38:34 UTC
When using the overdue report circ/overdue.pl, if the TimeFormat system preference is set to 12-hour, setting a filter on the due date range can result in zero overdue loans being displayed, even if there are ones that fall in the range.

This happens because the date/time that gets passed to the SQL is formatted incorrectly; rather than '2016-07-29 23:59', it gets passed as '2016-07-29 11:59 PM'; on many MySQL and MariaDB versions, that becomes NULL when cast to a datetime.
Comment 1 Galen Charlton 2016-07-29 21:07:06 UTC Comment hidden (obsolete)
Comment 2 Jason Robb 2016-08-06 19:00:11 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2016-08-07 11:54:24 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2016-08-07 11:55:31 UTC
Created attachment 54120 [details] [review]
[PASSED QA] Bug 17001: fix due date filter on the overdue report

When the TimeFormat system preference is set to "12 hour",
setting a filter on the due date can result in:

- no overdue loans being reported, even if there are some
  that meet the criteria

OR

- overdue loans being omitted from the report if they
  are due on the "until" date in the filter

This patch fixes this by replacing output_pref() with
DateTime::Format::MySQL to format the date filter values
to pass to the SQL query.

To test
-------
[1] Run the overdue report (circ/overdue.pl) and set a filter
    on due date, using values that should bring up one or
    more overdue loans.
[2] Note that zero overdue loans are returned (if using MySQL
    5.5, 5.6, or 5.7 or MariaDB 5) or that loans due on the
    "until" date are omitted (if using MarioDB 10).
[3] Apply the patch and repeat step 1. This time, the correct
    set of overdue loans should be reported.

Signed-off-by: Galen Charlton <gmcharlt@gmail.com>

Signed-off-by: Jason Robb <jrobb@sekls.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 5 Katrin Fischer 2016-08-07 11:57:19 UTC
Hi Jason, fixed your sign-off line to have your name - you might want to fix your git config :)
Comment 6 Kyle M Hall 2016-08-10 13:47:46 UTC
Would it not be simpler to pass the parameter timeformat => '24hr' to output_pref instead of using an external lib?
Comment 7 Galen Charlton 2016-08-11 14:02:47 UTC
(In reply to Kyle M Hall from comment #6)
> Would it not be simpler to pass the parameter timeformat => '24hr' to
> output_pref instead of using an external lib?

I note that output_pref(), if we were to take its name literally, is for formatting date and time *output*. DateTime::Format::MySQL is already used in other places and its name makes it clear that a DateTime passed through is meant to be fed to an SQL query.

I think it better that we keep the concerns of formatting timestamps for human display separate from formatting them for computer consumption.

I am reseting the status of this bug back to "Passed QA".
Comment 8 Kyle M Hall 2016-08-18 16:10:56 UTC
That is sufficient to me! Pushed to master for 16.11!

(In reply to Galen Charlton from comment #7)
> (In reply to Kyle M Hall from comment #6)
> > Would it not be simpler to pass the parameter timeformat => '24hr' to
> > output_pref instead of using an external lib?
> 
> I note that output_pref(), if we were to take its name literally, is for
> formatting date and time *output*. DateTime::Format::MySQL is already used
> in other places and its name makes it clear that a DateTime passed through
> is meant to be fed to an SQL query.
> 
> I think it better that we keep the concerns of formatting timestamps for
> human display separate from formatting them for computer consumption.
> 
> I am reseting the status of this bug back to "Passed QA".
Comment 9 Frédéric Demians 2016-08-23 10:42:41 UTC
Pushed in 16.05. Will be in 16.05.03.