Bug 28585

Summary: Cannot search on date fields
Product: Koha Reporter: Hannamari Heiniluoma <hannamari.h.heiniluoma>
Component: REST APIAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: major    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, kyle, martin.renvoize, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28632
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29157
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This patch fixes the date handling for query parsing from the API. We use dt_from_string to convert out RFC3339 formatted date strings to DateTime objects with an associated timezone and then user the native datetime formatted provided by the SQL connection library to convert to an appropriately formated date time string.
Version(s) released in:
21.11.00,21.05.05
Attachments: Bug 28585: Fix search on date field
Bug 28585: Regression tests
Bug 28585: Use the datetime_parser for handling API dates
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters
Bug 28585: Regression tests
Bug 28585: Use the datetime_parser for handling API dates
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters
Bug 28585: Regression tests
Bug 28585: Use the datetime_parser for handling API dates
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters
Bug 28585: Regression tests
Bug 28585: Use the datetime_parser for handling API dates
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters
Bug 28585: (QA follow-up) Fix object.t
Bug 28585: (QA follow-up) Add explicit tests for setting date/date-time attributes

Description Hannamari Heiniluoma 2021-06-17 11:27:56 UTC
Querying patrons by their date_of_birth returns an empty response, even though there are patrons with that date of birth in the database. Might be a bug?
Comment 1 Katrin Fischer 2021-06-19 11:39:36 UTC
Hi Hanna, could you include a sample for your query? Which date format was used?
Comment 2 Katrin Fischer 2021-06-19 11:39:58 UTC
(In reply to Katrin Fischer from comment #1)
> Hi Hanna, could you include a sample for your query? Which date format was
> used?

Sorry, Hannamari!
Comment 3 Hannamari Heiniluoma 2021-06-21 05:26:08 UTC
Hi!

My query was /patrons/?date_of_birth=2000-01-01. That returns status code 200 and an empty response []. If I change the date format, eg. 
/patrons/?date_of_birth=2000/01/01 (or any other format I can think of), I get 500 and 
{
    "error": "Something went wrong, check Koha logs for details."
}
Comment 4 Jonathan Druart 2021-06-24 10:44:10 UTC
DBIC is receiving
    dateofbirth   {                            
        like   "%1980-04-24T00:00:00%"                                                  
    }
Comment 5 Jonathan Druart 2021-06-24 10:47:34 UTC
Created attachment 122381 [details] [review]
Bug 28585: Fix search on date field

We should not rfc3339 format the date if it's not a datetime
Comment 6 Jonathan Druart 2021-06-24 10:48:00 UTC
Tomas, looks like this could be a fix. Do you see something else?
Comment 7 Tomás Cohen Arazi 2021-06-25 13:00:14 UTC
(In reply to Jonathan Druart from comment #6)
> Tomas, looks like this could be a fix. Do you see something else?

I wrote regression tests and this doesn't work, because it fails for date-time parameters. I'll take care.
Comment 8 Tomás Cohen Arazi 2021-06-25 14:53:21 UTC
Created attachment 122444 [details] [review]
Bug 28585: Regression tests

This patch adds tests for filtering GET calls with date/date-time
parameters. Tests fail because the feature is not working

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2021-06-25 14:53:25 UTC
Created attachment 122445 [details] [review]
Bug 28585: Use the datetime_parser for handling API dates

This patchset takes the GET /patrons route as a sample usage for filtering
on date and date-time (including timestamp) fields.

It then makes Koha::Object->attributes_from_api use the DB storage
datetime parser for format the parameters correctly.

To test:

1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: It doesn't find the patron when filtering by date
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works now!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2021-06-25 14:53:29 UTC
Created attachment 122446 [details] [review]
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2021-06-25 15:17:03 UTC
I made the q= tests a follow-up, because I'm not sure bug 28480 will be backported to affected stable branches, and so ease backporting this important fix.
Comment 12 David Nind 2021-06-25 23:32:35 UTC
Created attachment 122457 [details] [review]
Bug 28585: Regression tests

This patch adds tests for filtering GET calls with date/date-time
parameters. Tests fail because the feature is not working

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2021-06-25 23:32:39 UTC
Created attachment 122458 [details] [review]
Bug 28585: Use the datetime_parser for handling API dates

This patchset takes the GET /patrons route as a sample usage for filtering
on date and date-time (including timestamp) fields.

It then makes Koha::Object->attributes_from_api use the DB storage
datetime parser for format the parameters correctly.

To test:

1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: It doesn't find the patron when filtering by date
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works now!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2021-06-25 23:32:43 UTC
Created attachment 122459 [details] [review]
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 Martin Renvoize 2021-07-12 15:24:48 UTC
I think this will need some of the handling from Bug 24850 to work properly across timezones.
Comment 16 Kyle M Hall 2021-07-23 13:50:41 UTC
Created attachment 123115 [details] [review]
Bug 28585: Regression tests

This patch adds tests for filtering GET calls with date/date-time
parameters. Tests fail because the feature is not working

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2021-07-23 13:51:00 UTC
Created attachment 123116 [details] [review]
Bug 28585: Use the datetime_parser for handling API dates

This patchset takes the GET /patrons route as a sample usage for filtering
on date and date-time (including timestamp) fields.

It then makes Koha::Object->attributes_from_api use the DB storage
datetime parser for format the parameters correctly.

To test:

1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: It doesn't find the patron when filtering by date
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works now!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2021-07-23 13:51:04 UTC
Created attachment 123117 [details] [review]
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Jonathan Druart 2021-07-26 08:43:19 UTC
Please add something for the release notes as well as on the wiki page.
Comment 20 Jonathan Druart 2021-07-26 09:24:05 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 21 Jonathan Druart 2021-07-26 10:41:59 UTC
t/db_dependent/Koha/Object.t .. 9/21 
    #   Failed test 'Given a string, a timestamp field is converted into a DateTime object'
    #   at t/db_dependent/Koha/Object.t line 514.
    #          got: ''
    #     expected: 'DateTime'

    #   Failed test 'Given a string, a datetime field is converted into a DateTime object'
    #   at t/db_dependent/Koha/Object.t line 528.
    #          got: ''
    #     expected: 'DateTime'

    #   Failed test 'Given a string, a date field is converted into a DateTime object'
    #   at t/db_dependent/Koha/Object.t line 542.
    #          got: ''
    #     expected: 'DateTime'
    # Looks like you failed 3 tests of 12.

#   Failed test 'attributes_from_api() tests'
#   at t/db_dependent/Koha/Object.t line 576.


Tests added by 
  commit badbd7d85913749e33d9bdba397dc79836080719
  Bug 23893: Catch dt_from_string exceptions                                                         


Looks like we should remove the tests, do you have something else to suggest?
Comment 22 Jonathan Druart 2021-07-26 10:43:50 UTC
Can you also remind me why we are not using Koha::DateUtils instead of $dtf?
Is that because of bug 24850? If so, shouldn't we add a comment about replacing the line when 24850 will be pushed?
Comment 23 Jonathan Druart 2021-07-30 05:23:11 UTC
Patches reverted.
Comment 24 Martin Renvoize 2021-10-21 15:18:47 UTC
Created attachment 126668 [details] [review]
Bug 28585: Regression tests

This patch adds tests for filtering GET calls with date/date-time
parameters. Tests fail because the feature is not working

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2021-10-21 15:18:51 UTC
Created attachment 126669 [details] [review]
Bug 28585: Use the datetime_parser for handling API dates

This patchset takes the GET /patrons route as a sample usage for filtering
on date and date-time (including timestamp) fields.

It then makes Koha::Object->attributes_from_api use the DB storage
datetime parser for format the parameters correctly.

To test:

1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: It doesn't find the patron when filtering by date
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works now!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2021-10-21 15:18:55 UTC
Created attachment 126670 [details] [review]
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2021-10-21 15:19:00 UTC
Created attachment 126671 [details] [review]
Bug 28585: (QA follow-up) Fix object.t

Object.t was still testing for the return of DateTime objects from
attributes_from_api. I checked all calls to attrbutes_from_api for
reliance of DateTime objects and confirmed they all get passed into
search calls and thus are better served as SQL formatted strings.

I then converted the test to check that the rfc3339 formatted dates
passed in were converted to strings appropriate for feeding into SQL
where statements instead.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2021-10-21 15:22:04 UTC
(In reply to Jonathan Druart from comment #22)
> Can you also remind me why we are not using Koha::DateUtils instead of $dtf?
> Is that because of bug 24850? If so, shouldn't we add a comment about
> replacing the line when 24850 will be pushed?

It's pretty standard practice to use the DateTime parser returned by DBI for the connected database to format DateTime objects for SQL... I'm not even sure what function does the same in DateUtils?

Personally, I've used the dtf method all over the codebase.. it's what I reach for first when inputting a query.
Comment 29 Jonathan Druart 2021-10-21 15:42:54 UTC
FTR, discussed on IRC.

Seeing set_from_api calling attributes_from_api I hadn't notice we were doing something else than insert/update (and DBIC accepts DT object).

Here the method deals with search as well and so needs to format.
Comment 30 Jonathan Druart 2021-10-21 15:45:27 UTC
Are we certain we have a test for the "set a date using the REST API" case?
Comment 31 Martin Renvoize 2021-10-21 15:46:42 UTC
(In reply to Jonathan Druart from comment #30)
> Are we certain we have a test for the "set a date using the REST API" case?

I can't find one.. I'll look at writing one (or begging Tomas to write one) tomorrow.. brain is dead for today already ;)
Comment 32 Tomás Cohen Arazi 2021-10-21 15:54:24 UTC
Any PUT test on patrons is testing that implicitly. But it wouldn't hurt to add some explicit tests.

I'll take care in a bit.
Comment 33 Tomás Cohen Arazi 2021-10-21 18:36:06 UTC
Created attachment 126679 [details] [review]
Bug 28585: (QA follow-up) Add explicit tests for setting date/date-time attributes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 34 Tomás Cohen Arazi 2021-10-21 18:43:34 UTC
I noticed I wrote this comment too fast, and maybe someone reading this back would benefit from a more detailed explanation, sorry for that :-D

(In reply to Tomás Cohen Arazi from comment #32)
> Any PUT test on patrons is testing that implicitly.

^^ This tried to mean that:

```
  my $json = $builder->build_object({ class => 'Koha::Patrons' })->to_api;
```

will have any date/date-time attribute filled with a random date/date-time (that's what TestBuilder does, and to_api converts to the correct format), which the POST and PUT tests already test when they do things like this:

```
   is_deeply($got, $newpatron, 'Returned patron from update matches expected');
```

Anyhow, we better have explicit tests for special things we want to highlight. Good catch, RM.
Comment 35 Martin Renvoize 2021-10-21 19:09:30 UTC
Ah, of course!  Nice explanation Tomas, I totally missed that deeply compare..

Glad to had an explicit test though too, just ensure we catch if should we ever change testbuilder in some way that reduces that implicit coverage.

Nice one
Comment 36 Jonathan Druart 2021-10-25 13:58:35 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 37 Kyle M Hall 2021-10-29 12:35:08 UTC
Pushed to 21.05.x for 21.05.05
Comment 38 Fridolin Somers 2021-11-10 23:33:27 UTC
I tried to apply on 20.11.x but tests fail.
Can you try to rebase please ?