2. GET /api/v1/jobs?q={"enqueued_on":{">":"2024-09-04T09:26:52-03:00"}} is generating the following query SELECT `me`.`id`, `me`.`status`, `me`.`progress`, `me`.`size`, `me`.`borrowernumber`, `me`.`type`, `me`.`queue`, `me`.`data`, `me`.`context`, `me`.`enqueued_on`, `me`.`started_on`, `me`.`ended_on` FROM `background_jobs` `me` WHERE `enqueued_on` > '2024-09-04T09:26:52-03:00' But when you run it manually, you get a warning: | Warning | 1292 | Truncated incorrect datetime value: '2024-09-04T09:26:52-03:00' |
Created attachment 171389 [details] [review] Bug 37902: Add tests
Created attachment 171390 [details] [review] Bug 37902: Make sure filtered_params are converted Previously we only adjusted the attributes that were passed directly, not the ones in 'q' (which is the recommended way now)
Created attachment 171391 [details] [review] Bug 37902: Make sure we loop over if a structure is passed
Created attachment 171392 [details] [review] Bug 37902: TODOs There are still different structures we won't handle properly. This patch adds conditionals to prevent failures or warnings. Should be done, but later.
Note that t/db_dependent/api/v1/checkouts.t is failing if the timezone in conf is not empty. It was failing before and still after after. The test is wrong (it does not mock the config).
No, it does not work...
Created attachment 171402 [details] [review] Bug 37902: Adjust timezone We didn't take into account the server's timezone. This does not feel right, we are not supposed to deal with time zone outside of Koha::DateUtils.
(In reply to Jonathan Druart from comment #7) > Created attachment 171402 [details] [review] [review] > Bug 37902: Adjust timezone > > We didn't take into account the server's timezone. > > This does not feel right, we are not supposed to deal with time zone > outside of Koha::DateUtils. After retesting bug 37905 I noticed that the patch was wrong, and so something was missing here. However I don't understand why the tests were passing without!
Created attachment 171403 [details] [review] Bug 37902: Adjust timezone We didn't take into account the server's timezone. This does not feel right, we are not supposed to deal with time zone outside of Koha::DateUtils.
There are failures t/db_dependent/api/v1/patrons.t .......... # Failed test '200 OK' # at t/db_dependent/api/v1/patrons.t line 173. # got: '500' # expected: '200' # Failed test 'Filtering by date works' # at t/db_dependent/api/v1/patrons.t line 173. # got: undef # expected: '179' # Failed test '200 OK' # at t/db_dependent/api/v1/patrons.t line 177. # got: '500' # expected: '200' # Failed test 'Filtering by date-time works' # at t/db_dependent/api/v1/patrons.t line 177. # got: undef # expected: '179' # Looks like you failed 4 tests of 12.
(In reply to Jonathan Druart from comment #10) > There are failures > > t/db_dependent/api/v1/patrons.t .......... > # Failed test '200 OK' > # at t/db_dependent/api/v1/patrons.t line 173. > # got: '500' > # expected: '200' This is a bad one. Invalid date format: %1980-06-18% at /kohadevbox/koha/Koha/REST/Plugin/Objects.pm line 288 Because we are now hitting Koha/REST/Plugin/Query.pm 195 my $match = $reserved_params->{_match} // 'contains'; 196 197 foreach my $param ( keys %{$filtered_params} ) { 198 if ( $match eq 'contains' ) { 199 $params->{$param} = 200 { like => '%' . $filtered_params->{$param} . '%' };
Created attachment 171515 [details] [review] Bug 37902: Do not convert a date if it has like markers If we pass a datetime parameter we are adding "like" operator and % at the beginning and ending of the attribute value. For instance: attributes=2024-09-16 10:11:12 attributes:{ like => '%2024-09-16 10:11:12%' } We do not want to reach the fixup code and raise an exception. However I don't think we should add the like for datetime attributes actually. But can we modify this behaviour now?
There is still (at least) one failing test: # Failed test 'Filtering by date-time works' # at t/db_dependent/api/v1/patrons.t line 177. # got: undef # expected: '401' # Looks like you failed 1 test of 12. I don't think we are processing the values correctly. Why do we add like operator for dates? Do we want to keep this behaviour? I really need feedback and review on those patches before continuing working on this.
Created attachment 171551 [details] [review] Bug 37902: Add tests
(In reply to Jonathan Druart from comment #14) > Created attachment 171551 [details] [review] [review] > Bug 37902: Add tests This is fixing Martin's failure (401 unauthorized), I had this set in my custom.sql. Sorry! And kudos to Tomas who found this!
(In reply to Jonathan Druart from comment #13) > There is still (at least) one failing test: > > # Failed test 'Filtering by date-time works' > # at t/db_dependent/api/v1/patrons.t line 177. > # got: undef > # expected: '401' > # Looks like you failed 1 test of 12. > > I don't think we are processing the values correctly. Why do we add like > operator for dates? Do we want to keep this behaviour? > > I really need feedback and review on those patches before continuing working > on this. I think the like search for dates would only be used for partial date search in a column - i.e. "2024-07" - but we could probably replace that with year/month/day dropdowns - or just a datepicker and have reports for more complicated date searching?
(In reply to Nick Clemens (kidclamp) from comment #16) > (In reply to Jonathan Druart from comment #13) > > There is still (at least) one failing test: > > > > # Failed test 'Filtering by date-time works' > > # at t/db_dependent/api/v1/patrons.t line 177. > > # got: undef > > # expected: '401' > > # Looks like you failed 1 test of 12. > > > > I don't think we are processing the values correctly. Why do we add like > > operator for dates? Do we want to keep this behaviour? > > > > I really need feedback and review on those patches before continuing working > > on this. > > I think the like search for dates would only be used for partial date search > in a column - i.e. "2024-07" - but we could probably replace that with > year/month/day dropdowns - or just a datepicker and have reports for more > complicated date searching? Yes, but this could be done client-side. If we are passing an invalid/incomplete date then we can build the "like" part ourselves.
(In reply to Nick Clemens (kidclamp) from comment #16) > (In reply to Jonathan Druart from comment #13) > > There is still (at least) one failing test: > > > > # Failed test 'Filtering by date-time works' > > # at t/db_dependent/api/v1/patrons.t line 177. > > # got: undef > > # expected: '401' > > # Looks like you failed 1 test of 12. > > > > I don't think we are processing the values correctly. Why do we add like > > operator for dates? Do we want to keep this behaviour? > > > > I really need feedback and review on those patches before continuing working > > on this. > > I think the like search for dates would only be used for partial date search > in a column - i.e. "2024-07" - but we could probably replace that with > year/month/day dropdowns - or just a datepicker and have reports for more > complicated date searching? Flagging this comment as important, it's actually a TODO for some DT searches.
(In reply to Jonathan Druart from comment #17) > Yes, but this could be done client-side. If we are passing an > invalid/incomplete date then we can build the "like" part ourselves. Good point, we could skip autotruncation for date fields and have the user add them if needed
Created attachment 171652 [details] [review] Bug 37902: Apply exact match for datetime We do not want to apply "like" and do a "contains" search if a correctly formatted date is passed (ie. starting with "YYYY-MM-DD HH:MM:SS") It causes underlying problems if we add '%' characters to this string as it will then become an invalid date. There are several ways of dealing with this problem. This patch is suggesting the easiest path: Apply an exact search (ie. do not add '%') if the value appears to be a datetime. Certainly not the best looking patch but it seems to be quite effective: * no need to change the client * no need to rework build_query_params, merge_q_params, attributes_from_api We could (to confirm) pass the result set, but it seems a lot of additional processing (that is done later already, in attributes_from_api)
Created attachment 171762 [details] [review] Bug 37902: Make sure filtered_params are converted Previously we only adjusted the attributes that were passed directly, not the ones in 'q' (which is the recommended way now) Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 171763 [details] [review] Bug 37902: Make sure we loop over if a structure is passed Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 171764 [details] [review] Bug 37902: TODOs There are still different structures we won't handle properly. This patch adds conditionals to prevent failures or warnings. Should be done, but later. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 171765 [details] [review] Bug 37902: Adjust timezone We didn't take into account the server's timezone. This does not feel right, we are not supposed to deal with time zone outside of Koha::DateUtils. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 171766 [details] [review] Bug 37902: Do not convert a date if it has like markers If we pass a datetime parameter we are adding "like" operator and % at the beginning and ending of the attribute value. For instance: attributes=2024-09-16 10:11:12 attributes:{ like => '%2024-09-16 10:11:12%' } We do not want to reach the fixup code and raise an exception. However I don't think we should add the like for datetime attributes actually. But can we modify this behaviour now? Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 171767 [details] [review] Bug 37902: Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 171768 [details] [review] Bug 37902: Apply exact match for datetime We do not want to apply "like" and do a "contains" search if a correctly formatted date is passed (ie. starting with "YYYY-MM-DD HH:MM:SS") It causes underlying problems if we add '%' characters to this string as it will then become an invalid date. There are several ways of dealing with this problem. This patch is suggesting the easiest path: Apply an exact search (ie. do not add '%') if the value appears to be a datetime. Certainly not the best looking patch but it seems to be quite effective: * no need to change the client * no need to rework build_query_params, merge_q_params, attributes_from_api We could (to confirm) pass the result set, but it seems a lot of additional processing (that is done later already, in attributes_from_api) Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Makes sense, tests pass, there are tidiness complaints from QA tools
So bug 36217 stopped us from passing a UTC timestamp like 2024-09-26T18:41:24.268Z to the API. This timestamp was generated by Date().toISOString(). Bug 37905 wants to use a RFC3339 timestamp like 2024-09-26T14:45:36-04:00 via dayjs().format()... and bug 37902 (this one) allows us to use RFC3339... If we're going to convert the incoming timestamp from the client... do we want to use RFC3339 or why not keep using ISO? Anyway, I'll look at this more later. Doing things in person with humans today and now the day is over...
David, if you are asking what's the next step then have a look at bug 37952.
Created attachment 172105 [details] [review] Bug 37902: Make sure filtered_params are converted Previously we only adjusted the attributes that were passed directly, not the ones in 'q' (which is the recommended way now) Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 172106 [details] [review] Bug 37902: Make sure we loop over if a structure is passed Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 172107 [details] [review] Bug 37902: TODOs There are still different structures we won't handle properly. This patch adds conditionals to prevent failures or warnings. Should be done, but later. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 172108 [details] [review] Bug 37902: Adjust timezone We didn't take into account the server's timezone. This does not feel right, we are not supposed to deal with time zone outside of Koha::DateUtils. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 172109 [details] [review] Bug 37902: Do not convert a date if it has like markers If we pass a datetime parameter we are adding "like" operator and % at the beginning and ending of the attribute value. For instance: attributes=2024-09-16 10:11:12 attributes:{ like => '%2024-09-16 10:11:12%' } We do not want to reach the fixup code and raise an exception. However I don't think we should add the like for datetime attributes actually. But can we modify this behaviour now? Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 172110 [details] [review] Bug 37902: Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 172111 [details] [review] Bug 37902: Apply exact match for datetime We do not want to apply "like" and do a "contains" search if a correctly formatted date is passed (ie. starting with "YYYY-MM-DD HH:MM:SS") It causes underlying problems if we add '%' characters to this string as it will then become an invalid date. There are several ways of dealing with this problem. This patch is suggesting the easiest path: Apply an exact search (ie. do not add '%') if the value appears to be a datetime. Certainly not the best looking patch but it seems to be quite effective: * no need to change the client * no need to rework build_query_params, merge_q_params, attributes_from_api We could (to confirm) pass the result set, but it seems a lot of additional processing (that is done later already, in attributes_from_api) Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 172112 [details] [review] Bug 37902: (QA follow-up) Tidy Query.pm & Object.t Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Jonathan Druart from comment #30) > David, if you are asking what's the next step then have a look at bug 37952. No, that's not what I meant, but you can ignore my earlier question, as I've done more digging and I think I understand better today than yesterday. If I understand correctly, Koha::DateTime::Format::RFC3339->parse_datetime() should correctly parse both 2024-09-26T18:41:24.268Z and 2024-09-26T14:45:36-04:00 and set the time_zone appropriately. I looked back through your patches and Koha::Object->attributes_from_api() pre-patch, so I better understand what bug you're trying to fix here. I haven't thoroughly reviewed and understood every line of code, but I get the general idea. -- I was wondering if there would be any negative unintended consequences of fixing this bug... but we should be OK anywhere that dayjs() with the RFC3339 output or Date().toISOString() is used. If no timezone is provided, it's just assumed that the server timezone is used, right? -- The % for the -like operator is still going to be a problem, but... I think that's a future problem, which will be part of bug 37952 anyway, as you say Jonathan, so I'll comment more there...
If it's tl;dr, ignore the above comment and just know that I think it's all good here :)