Bug 35284 - No more delay between 2 DT requests
Summary: No more delay between 2 DT requests
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 31199 35309 35396
  Show dependency treegraph
 
Reported: 2023-11-08 09:39 UTC by Jonathan Druart
Modified: 2023-11-24 14:13 UTC (History)
10 users (show)

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


Attachments
Bug 35284: [DO NOT PUSH] Add DT delay to kohaTable (2.73 KB, patch)
2023-11-08 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35284: Add DT delay to kohaTable (2.71 KB, patch)
2023-11-09 09:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35284: Fix Vue tables (2.17 KB, patch)
2023-11-09 09:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35284: Add throttling to column filters (1.44 KB, patch)
2023-11-09 09:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35284: Fix tests (10.78 KB, patch)
2023-11-09 09:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35284: Add DT delay to kohaTable (2.76 KB, patch)
2023-11-09 23:50 UTC, David Cook
Details | Diff | Splinter Review
Bug 35284: Fix Vue tables (2.22 KB, patch)
2023-11-09 23:50 UTC, David Cook
Details | Diff | Splinter Review
Bug 35284: Add throttling to column filters (1.49 KB, patch)
2023-11-09 23:50 UTC, David Cook
Details | Diff | Splinter Review
Bug 35284: Fix tests (10.83 KB, patch)
2023-11-09 23:50 UTC, David Cook
Details | Diff | Splinter Review
Bug 35284: Fix tests (10.92 KB, patch)
2023-11-10 08:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35284: Add DT delay to kohaTable (2.86 KB, patch)
2023-11-10 08:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35284: Fix Vue tables (2.31 KB, patch)
2023-11-10 08:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35284: Add throttling to column filters (1.59 KB, patch)
2023-11-10 08:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35284: Fix tests (10.92 KB, patch)
2023-11-10 08:15 UTC, Marcel de Rooy
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-11-08 09:39:17 UTC
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+.
"""
Comment 2 Jonathan Druart 2023-11-08 09:42:05 UTC
This can be (very) problematic for perfs.
Comment 3 Jonathan Druart 2023-11-08 21:33:07 UTC
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
Comment 4 Jonathan Druart 2023-11-08 21:42:39 UTC
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.
Comment 5 David Cook 2023-11-08 22:19:18 UTC
(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?
Comment 6 Jonathan Druart 2023-11-09 09:03:17 UTC
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
Comment 7 Jonathan Druart 2023-11-09 09:03:20 UTC
Created attachment 158706 [details] [review]
Bug 35284: Fix Vue tables
Comment 8 Jonathan Druart 2023-11-09 09:03:23 UTC
Created attachment 158707 [details] [review]
Bug 35284: Add throttling to column filters
Comment 9 Jonathan Druart 2023-11-09 09:33:39 UTC
So now the selenium tests are failing because the ajax request does not exist yet and so we are not waiting for it, great...
Comment 10 Jonathan Druart 2023-11-09 09:46:26 UTC
Created attachment 158708 [details] [review]
Bug 35284: Fix tests

This fix patrons_search.t but other selenium tests certainly need
adjustements
Comment 11 David Nind 2023-11-09 13:52:50 UTC
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?
Comment 12 Jonathan Druart 2023-11-09 14:19:45 UTC
(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).
Comment 13 Jonathan Druart 2023-11-09 14:25:42 UTC
I got the tests (selenium/patrons_search.t) passed 200x in a row.
Comment 14 David Cook 2023-11-09 23:04:25 UTC
(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.
Comment 15 David Cook 2023-11-09 23:37:48 UTC
(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.
Comment 16 David Cook 2023-11-09 23:49:15 UTC
(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
Comment 17 David Cook 2023-11-09 23:50:03 UTC
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>
Comment 18 David Cook 2023-11-09 23:50:06 UTC
Created attachment 158777 [details] [review]
Bug 35284: Fix Vue tables

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 19 David Cook 2023-11-09 23:50:09 UTC
Created attachment 158778 [details] [review]
Bug 35284: Add throttling to column filters

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 20 David Cook 2023-11-09 23:50:13 UTC
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>
Comment 21 David Cook 2023-11-10 00:08:27 UTC
(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
Comment 22 Marcel de Rooy 2023-11-10 07:39:38 UTC
QA: Looking here
Comment 23 Marcel de Rooy 2023-11-10 08:14:20 UTC
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>
Comment 24 Marcel de Rooy 2023-11-10 08:15:27 UTC
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>
Comment 25 Marcel de Rooy 2023-11-10 08:15:30 UTC
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>
Comment 26 Marcel de Rooy 2023-11-10 08:15:32 UTC
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>
Comment 27 Marcel de Rooy 2023-11-10 08:15:35 UTC
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>
Comment 28 Marcel de Rooy 2023-11-10 08:21:24 UTC
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".
Comment 29 Marcel de Rooy 2023-11-10 08:27:04 UTC
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..
Comment 30 Jonathan Druart 2023-11-10 08:36:36 UTC
(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.
Comment 31 Jonathan Druart 2023-11-10 08:37:08 UTC
(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.
Comment 32 Jonathan Druart 2023-11-10 08:41:35 UTC
And finally, see also bug 35309.
Comment 33 Tomás Cohen Arazi 2023-11-10 14:02:48 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 34 Fridolin Somers 2023-11-11 07:47:00 UTC
Pushed to 23.05.x for 23.05.06
Comment 35 Jonathan Druart 2023-11-14 15:43:23 UTC
Pedro, why not backporting this one?
Comment 36 Pedro Amorim 2023-11-14 17:30:07 UTC
(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.
Comment 37 Lari Strand 2023-11-23 14:14:36 UTC
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.
Comment 38 David Cook 2023-11-23 23:29:25 UTC
(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.
Comment 39 Jonathan Druart 2023-11-24 10:51:03 UTC
(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.
Comment 40 Lari Strand 2023-11-24 12:41:22 UTC
I added my proposition to https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35396