Bug 34226 - Format dates from DT filters before querying the REST API
Summary: Format dates from DT filters before querying the REST API
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
: 32559 (view as bug list)
Depends on: 22440
Blocks: 33568 34352
  Show dependency treegraph
 
Reported: 2023-07-07 13:08 UTC by Jonathan Druart
Modified: 2023-12-28 20:47 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 34226: DT wrapper - pre-processed date term when filtering (2.54 KB, patch)
2023-07-07 13:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34226: DT wrapper - pre-processed date term when filtering (2.60 KB, patch)
2023-07-24 10:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34226: WIP Using 'type' at the datatable config level (3.42 KB, patch)
2023-07-24 10:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34226: Remove apply_flatpickr (2.64 KB, patch)
2023-07-24 11:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34226: DT wrapper - pre-processed date term when filtering (2.71 KB, patch)
2023-07-25 08:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34226: Use 'type' at the datatable config level (6.65 KB, patch)
2023-07-25 08:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34226: Add tests (9.14 KB, patch)
2023-07-25 08:16 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-07-07 13:08:07 UTC
If we have filters on top of column on a table that is using the DT REST API wrapper, we cannot filter on date using formatted dates.
Comment 1 Jonathan Druart 2023-07-07 13:15:54 UTC
Created attachment 153180 [details] [review]
Bug 34226: DT wrapper - pre-processed date term when filtering

If we have filters on top of column on a table that is using the DT REST API wrapper,
we cannot filter on date using formatted dates.

This was done for "date of birth" for bug 32505.
Here we want to provide a generic approach.

Note that we cannot use what has been done on bug 22440 in some cases
(when we don't write the thead DOM directly but rely on DataTables
constructor, for instance bug 33568). The data- attributes are not
passed by DT.

Test plan:
On top of 33568, filter date columns using the full version of the
formatted date
Comment 2 Martin Renvoize 2023-07-24 10:15:59 UTC
Created attachment 153830 [details] [review]
Bug 34226: DT wrapper - pre-processed date term when filtering

If we have filters on top of column on a table that is using the DT REST API wrapper,
we cannot filter on date using formatted dates.

This was done for "date of birth" for bug 32505.
Here we want to provide a generic approach.

Note that we cannot use what has been done on bug 22440 in some cases
(when we don't write the thead DOM directly but rely on DataTables
constructor, for instance bug 33568). The data- attributes are not
passed by DT.

Test plan:
On top of 33568, filter date columns using the full version of the
formatted date

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2023-07-24 10:16:02 UTC
Created attachment 153831 [details] [review]
Bug 34226: WIP Using 'type' at the datatable config level

What about using 'type' definitions at the datatables settings level and
binding that to a flatpickr instance?
Comment 4 Martin Renvoize 2023-07-24 10:21:22 UTC
I struggled a bit to put this in context with bug 33568 so I was testing it using the patrons search table (yes, I know.. it's already one of the more complex ones)

I've added a patch on top that takes us towards a more generic way of displaying the standard flatpickr in the search headers.. does this go towards achieving what you hope or am I completely misunderstanding the work here?
Comment 5 Jonathan Druart 2023-07-24 11:05:22 UTC
Thanks Martin, that's definitely the way to go. I thought I tried it and it didn't work, but apparently I was wrong.

There is only thing however that would not work directly: the apply_flatpickr. It is in calendar.inc and you have to include it from the tt you are including datatables.js
I've tried with 33568 and it does not work (at least easily) because of the way includes and js files are embedded. I would move the flatpickr thing to another bug report.
Comment 6 Jonathan Druart 2023-07-24 11:11:24 UTC
There is another thing with flatpickr: you cannot longer search for a year. If you want to search all patrons born in 1958, flatpickr will blank the input if no date has been selected.
Comment 7 Jonathan Druart 2023-07-24 11:14:18 UTC
Created attachment 153835 [details] [review]
Bug 34226: Remove apply_flatpickr

It needs more work. The include calendar.inc should not be done in
patron-search.inc, and the order of the include (inc+js) is wrong at
some point (need more investigation)

Also you cannot search for a specific year, you need to enter a valid
date
Comment 8 Jonathan Druart 2023-07-24 11:19:59 UTC
I've updated and adjusted the remote branch of bug 33568 with your suggestion and it is working great.
Comment 9 Martin Renvoize 2023-07-24 11:32:57 UTC
Excellent, thanks for the feedback Jonathan, I'm glad I could help here.

Yes, more than happy to migrate the flatpickr piece into it's own bug, that's certainly where the WIP was headed.. I agree, the order of inserting the JS is a bit funky and I'd like to somehow improve that but got a bit stuck there in the quick look I had.  As for year only, I also spotted that and wasn't yet sure how to overcome it.

I'll open another bug for the flatpickr side.. it's a lovely addition if we can get it working for the cases we need.
Comment 10 Martin Renvoize 2023-07-24 14:25:30 UTC
Oh.. isn't this also an almost duplicate of bug 32559 ?
Comment 11 Martin Renvoize 2023-07-24 15:31:30 UTC
*** Bug 32559 has been marked as a duplicate of this bug. ***
Comment 12 Jonathan Druart 2023-07-24 15:38:40 UTC
Yes, definitely a duplicate!
Comment 13 Martin Renvoize 2023-07-24 17:26:25 UTC
Will be submitting the squash of both bugs first thing in the morning.
Comment 14 Martin Renvoize 2023-07-25 08:16:35 UTC
Created attachment 153862 [details] [review]
Bug 34226: DT wrapper - pre-processed date term when filtering

If we have filters on top of column on a table that is using the DT REST API wrapper,
we cannot filter on date using formatted dates.

This was done for "date of birth" for bug 32505.
Here we want to provide a generic approach.

Note that we cannot use what has been done on bug 22440 in some cases
(when we don't write the thead DOM directly but rely on DataTables
constructor, for instance bug 33568). The data- attributes are not
passed by DT.

Test plan:
On top of 33568, filter date columns using the full version of the
formatted date

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2023-07-25 08:16:37 UTC
Created attachment 153863 [details] [review]
Bug 34226: Use 'type' at the datatable config level

Lets use 'type' definitions at the datatables settings level instead

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2023-07-25 08:16:40 UTC
Created attachment 153864 [details] [review]
Bug 34226: Add tests

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2023-07-25 08:26:09 UTC
Bugs merged, tests stolen, WIP commit and follow-up squashed and all submitted here.. signoffs migrated and QA scripts all run for good measure again.

All is working well from my perspective, Passing QA
Comment 18 Jonathan Druart 2023-07-25 08:55:22 UTC
Applied these patches on the remote branch of bug 33568 and all seems to work perfectly. Thanks, Martin!
Comment 19 Tomás Cohen Arazi 2023-07-25 19:26:27 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 20 Fridolin Somers 2023-08-04 02:24:01 UTC
Enhancement not backported to 23.05.x