Coming from bug 31199 comment 16 We expect a 250ms delay between request, it is in koha-tmpl/intranet-tmpl/prog/js/datatables.js 114 jQuery.fn.dataTableExt.oApi.fnSetFilteringDelay = function ( oSettings, iDelay ) { But, the from doc: https://datatables.net/plug-ins/api/fnSetFilteringDelay """ Please note that this plug-in has been deprecated and the dt-init searchDelay option in DataTables 1.10 should now be used. This plug-in will not operate with v1.10+. """
There are workarounds on https://datatables.net/forums/discussion/33028/searchdelay-for-server-side-issue https://www.datatables.net/forums/discussion/comment/113264
This can be (very) problematic for perfs.
Created attachment 158676 [details] [review] Bug 35284: [DO NOT PUSH] Add DT delay to kohaTable To test: * When a specific initComplete exists (general patron search) * When no specific initComplete exists (cities) => KO * ERM tables display twice the "show X entries" dropdown list * Column filters are not affected
IMO we should just stop the hack and use the searchDelay provided by DataTables. If it's not enough then we should consider sponsoring the enhancement to DataTables itself.
(In reply to Jonathan Druart from comment #4) > IMO we should just stop the hack and use the searchDelay provided by > DataTables. > > If it's not enough then we should consider sponsoring the enhancement to > DataTables itself. Sounds like a plan. Let's take a look at adding searchDelay?
Created attachment 158705 [details] [review] Bug 35284: Add DT delay to kohaTable To test: * When a specific initComplete exists (general patron search) * When no specific initComplete exists (cities) => KO * ERM tables display twice the "show X entries" dropdown list * Column filters are not affected
Created attachment 158706 [details] [review] Bug 35284: Fix Vue tables
Created attachment 158707 [details] [review] Bug 35284: Add throttling to column filters
So now the selenium tests are failing because the ajax request does not exist yet and so we are not waiting for it, great...
Created attachment 158708 [details] [review] Bug 35284: Fix tests This fix patrons_search.t but other selenium tests certainly need adjustements
Not sure how to test. The tests pass before and after the patch is applied (prove t/db_dependent/selenium/patrons_search.t - with ktd started with ktd --selenium up). Is that sufficient for signing off?
(In reply to David Nind from comment #11) > Not sure how to test. > > The tests pass before and after the patch is applied (prove > t/db_dependent/selenium/patrons_search.t - with ktd started with ktd > --selenium up). > > Is that sufficient for signing off? Please test the global search and the column filtering on the different tables: 1. patrons search (specific initComplete) 2. cities (no specific initComplete) 3. tables in Vue apps: ERM or preservation modules (using KohaTable.vue) 4. Other tables not using the REST API (should not be using by the delay) The delay is 500ms. The global search should hit a search (refresh of the table) 500ms after you hit the last key. The column filtering hit a search every 500ms (no matter if you are still typing).
I got the tests (selenium/patrons_search.t) passed 200x in a row.
(In reply to Jonathan Druart from comment #13) > I got the tests (selenium/patrons_search.t) passed 200x in a row. With these patches, that test is looking much better. Not hammering the server like before.
(In reply to Jonathan Druart from comment #12) > Please test the global search and the column filtering on the different > tables: > 1. patrons search (specific initComplete) > 2. cities (no specific initComplete) > 3. tables in Vue apps: ERM or preservation modules (using KohaTable.vue) > 4. Other tables not using the REST API (should not be using by the delay) > > The delay is 500ms. The global search should hit a search (refresh of the > table) 500ms after you hit the last key. > The column filtering hit a search every 500ms (no matter if you are still > typing). 1. http://localhost:8081/cgi-bin/koha/members/member.pl The global patron search autocomplete seems good. I type "koh" and it just does 1 search. The column filtering on the patron search seems OK. When I rapidly write "koha" in the Name column filter, it does 2 searches instead of 1. That's better than 4 though. 2. http://localhost:8081/cgi-bin/koha/admin/cities.pl I create a couple cities named "test1", "test2", etc I column filter by "City" rapidly writing "test" and it does 2 searches instead of 1. But still better than 4. 3. I turn on ERM and go to http://localhost:8081/cgi-bin/koha/erm/agreements I create a couple agreements named "test1", "test2", etc. Same column filtering as above. 4. Go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=134&searchid=scs_1699572642146 Click "Activate filters" Filters work correctly and quickly. -- I've done a little test and I should be writing "koha" and "test" in about 350ms. So with the column filtering I'm assuming it's doing 1 search immediately (the first search usually includes the first 1-2 keys struck) and then the next search is done 500ms later, which is after I've finished typing the full word... If I type the word "test" at a more normal speed, it's doing 4 searches. Testing against master... Cities: Same number of requests as with the patches. Patrons: When I rapidly type "koha", it does 3 searches consistently instead of the 2 searches that I get with the patches. -- So I'm not sure there's much improvement for human searching, but these patches do significantly help with running unit tests. It's like night and day.
(In reply to Jonathan Druart from comment #13) > I got the tests (selenium/patrons_search.t) passed 200x in a row. Unfortunately, it looks like a different change is now causing subtest 22 "Searching on a searchable attribute returns correct results" in this test script to fail in master, which makes this one harder to verify overall. But I think I'm happy enough to Sign Off with what I'm seeing (or rather not seeing) in plack.log
Created attachment 158776 [details] [review] Bug 35284: Add DT delay to kohaTable To test: * When a specific initComplete exists (general patron search) * When no specific initComplete exists (cities) => KO * ERM tables display twice the "show X entries" dropdown list * Column filters are not affected Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 158777 [details] [review] Bug 35284: Fix Vue tables Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 158778 [details] [review] Bug 35284: Add throttling to column filters Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 158779 [details] [review] Bug 35284: Fix tests This fix patrons_search.t but other selenium tests certainly need adjustements Signed-off-by: David Cook <dcook@prosentient.com.au>
(In reply to David Cook from comment #16) > (In reply to Jonathan Druart from comment #13) > > I got the tests (selenium/patrons_search.t) passed 200x in a row. > > Unfortunately, it looks like a different change is now causing subtest 22 > "Searching on a searchable attribute returns correct results" in this test > script to fail in master, which makes this one harder to verify overall. Looks like bug 34517
QA: Looking here
Created attachment 158783 [details] [review] Bug 35284: Fix tests This fix patrons_search.t but other selenium tests certainly need adjustements Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 158784 [details] [review] Bug 35284: Add DT delay to kohaTable To test: * When a specific initComplete exists (general patron search) * When no specific initComplete exists (cities) => KO * ERM tables display twice the "show X entries" dropdown list * Column filters are not affected Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 158785 [details] [review] Bug 35284: Fix Vue tables Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 158786 [details] [review] Bug 35284: Add throttling to column filters Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 158787 [details] [review] Bug 35284: Fix tests This fix patrons_search.t but other selenium tests certainly need adjustements Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Joubu told me just now that he is still working on the tests. Great. I would have added a comment about "other selenium tests certainly need adjustements".
This patch is just written yesterday and i understand the hurry to get it in still. This explains also the rather "poor" test plans ;) Just as David has observed, I see the effect of the change in the number of API calls to the server while typing in the search box. Must say that the whole DataTables stuff in Koha doesnt really attract me.. And instead of just adding a line for searchDelay we still need here 75 lines. Just mentioning this to illustrate that maintaining this setup becomes harder and harder..
(In reply to Marcel de Rooy from comment #29) > This patch is just written yesterday and i understand the hurry to get it in > still. This explains also the rather "poor" test plans ;) > > Just as David has observed, I see the effect of the change in the number of > API calls to the server while typing in the search box. > > Must say that the whole DataTables stuff in Koha doesnt really attract me.. > And instead of just adding a line for searchDelay we still need here 75 > lines. Just mentioning this to illustrate that maintaining this setup > becomes harder and harder.. The idea was to keep it as it was before. I don't really like the behaviour of searchDelay to be honest.
(In reply to Marcel de Rooy from comment #28) > Joubu told me just now that he is still working on the tests. Great. I would > have added a comment about "other selenium tests certainly need > adjustements". Other tests seem fine actually. The other one using wait_for_ajax is t/db_dependent/selenium/authentication_2fa.t but I don't manage to make it fail.
And finally, see also bug 35309.
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.06
Pedro, why not backporting this one?
(In reply to Jonathan Druart from comment #35) > Pedro, why not backporting this one? Thanks Jonathan, I've revisited and rebased it. Pushed to 22.11.x for 22.11.12.
I resolved the issue (in 22.11 without these patches) by setting a timeout in koha-tmpl/intranet-tmpl/prog/js/datatables.js that waits after typing has finished to the column filter field before filtering happens. This way we do not need to limit the amount of events fired (throttling) and the system feels responsive at all times, even with heavy filtering going on and lots of typing to the filter field. function delay(callback, ms) { var timer = 0; return function() { var context = this, args = arguments; clearTimeout(timer); timer = setTimeout(function () { callback.apply(context, args); }, ms || 0); }; } $( input_type, this ).on( 'keyup change', (delay(function () { if ( table_dt.column(i).search() !== this.value ) { if ( input_type == "input" ) { table_dt .column(i) .search( this.value ) .draw(); } else { table_dt .column(i) .search( this.value.length ? '^'+this.value+'$' : '', true, false ) .draw(); } } },500 ))); Something among those lines. 500ms at the bottom is a hardcoded wait time until the keyup or change event fires for which the value could be fetched from somewhere else is hard coding is an issue. Could this be a suitable/better approach still for filtering columns? I'm also wondering why we haven't updated the version of the Datatables libraries. Should it be updated? We have 4 years of commits missing. 1.13.8 seemed to work out of the box on my dev install of 22.11.
(In reply to Lari Strand from comment #37) > I'm also wondering why we haven't updated the version of the Datatables > libraries. Should it be updated? We have 4 years of commits missing. 1.13.8 > seemed to work out of the box on my dev install of 22.11. Yes, it should be updated. It's a big job which is still in progress.
(In reply to Lari Strand from comment #37) > I resolved the issue (in 22.11 without these patches) by setting a timeout > in koha-tmpl/intranet-tmpl/prog/js/datatables.js that waits after typing has > finished to the column filter field before filtering happens. This way we do > not need to limit the amount of events fired (throttling) and the system > feels responsive at all times, even with heavy filtering going on and lots > of typing to the filter field. If you think your solution is better, please open a separate bug report with a patch, describe why and the differences, and I will be happy to test. > I'm also wondering why we haven't updated the version of the Datatables > libraries. Should it be updated? We have 4 years of commits missing. 1.13.8 > seemed to work out of the box on my dev install of 22.11. Added you to the relevant bug.
I added my proposition to https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35396