|
Description
Hannamari Heiniluoma
2021-06-17 11:27:56 UTC
Hi Hanna, could you include a sample for your query? Which date format was used? (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! 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."
}
DBIC is receiving
dateofbirth {
like "%1980-04-24T00:00:00%"
}
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 Tomas, looks like this could be a fix. Do you see something else? (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. 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> 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> 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> 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. 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> 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> 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> I think this will need some of the handling from Bug 24850 to work properly across timezones. 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> 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> 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> Please add something for the release notes as well as on the wiki page. Pushed to master for 21.11, thanks to everybody involved! 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?
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? Patches reverted. 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> 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> 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> 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> (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. 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. Are we certain we have a test for the "set a date using the REST API" case? (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 ;) 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. 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> 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. 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 Pushed to master for 21.11, thanks to everybody involved! Pushed to 21.05.x for 21.05.05 I tried to apply on 20.11.x but tests fail. Can you try to rebase please ? |