Bug 36302 - Patron search from search bar broken
Summary: Patron search from search bar broken
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Pedro Amorim
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-03-12 16:45 UTC by Martin Renvoize (ashimema)
Modified: 2024-12-23 08:06 UTC (History)
8 users (show)

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


Attachments
Bug 36302: Remove defer_loading logic (2.80 KB, patch)
2024-03-12 17:48 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36302: Remove defer_loading logic (2.85 KB, patch)
2024-03-12 19:41 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 36302: Use replace instead of href (1.31 KB, patch)
2024-03-13 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36302: Remove unused code (1.47 KB, patch)
2024-03-13 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36302: Get rid of defer_loading (2.74 KB, patch)
2024-03-13 11:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36302: use replace instead of href (1.77 KB, patch)
2024-03-13 11:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36302: Revert back to POST (2.03 KB, patch)
2024-03-13 15:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36302: Revert back to POST (2.09 KB, patch)
2024-03-13 21:29 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 36302: Revert back to POST (2.08 KB, patch)
2024-03-13 21:45 UTC, David Nind
Details | Diff | Splinter Review
Bug 36302: [ALTERNATIVE] Get rid of defer_loading (2.70 KB, patch)
2024-03-15 11:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36302: [ALTERNATIVE] Get rid of defer_loading (2.75 KB, patch)
2024-03-15 12:26 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36302: Get rid of defer_loading in patron-search (2.83 KB, patch)
2024-03-20 05:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36302: Reinstate defer_loading (3.33 KB, patch)
2024-03-26 12:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36302: members-home.pl defers loading, member.pl does not (1.25 KB, patch)
2024-03-26 12:31 UTC, Pedro Amorim
Details | Diff | Splinter Review
bug 36302: Restore passing deferLoading to DT (940 bytes, patch)
2024-03-27 07:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
bug 36302: Restore passing deferLoading to DT (999 bytes, patch)
2024-03-27 13:42 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36302: Reinstate defer_loading (3.40 KB, patch)
2024-03-27 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36302: members-home.pl defers loading, member.pl does not (1.32 KB, patch)
2024-03-27 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
bug 36302: Restore passing deferLoading to DT (1.05 KB, patch)
2024-03-27 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize (ashimema) 2024-03-12 16:45:48 UTC
The patron search from the top bar has broken recently.  Search for Henry in ktd from the top bar, autocomplete works, but hitting return returns zero results (and doesn't redirect to the Henry user either any more)
Comment 1 Katrin Fischer 2024-03-12 16:47:01 UTC
Confirmed.
Comment 2 Pedro Amorim 2024-03-12 17:48:15 UTC
Created attachment 163096 [details] [review]
Bug 36302: Remove defer_loading logic

Test plan, ktd:
1) Search "henry" on the top search bar, notice 1 result is auto suggested
2) Ignore the auto-complete, press 'Enter' instead
3) Notice no results are returned.
4) Apply patch. Repeat.
Comment 3 Pedro Amorim 2024-03-12 17:50:17 UTC
This is a PoC highlighting and fixing the issue, but I'm unsure what that defer_loading code was doing int he first but was introduced by bug 30063 patch "Restore back behaviour".
@Joubu are you able to provide feedback here? Basically I believe my patch is fixing the issue but may be reintroducing past issues.
Comment 4 Pedro Amorim 2024-03-12 18:02:42 UTC
I believe this defer_loading check was added here to have the "back" browser behavior work in the event that a search only returned 1 result and the cgi->redirect to that result kicks in (as opposed to showing the search results with just 1 entry).
Is this cgi->redirect (in members/member.pl) still needed now that we have auto-complete suggestions?
Comment 5 Brendan Lawlor 2024-03-12 19:41:24 UTC
Created attachment 163099 [details] [review]
Bug 36302: Remove defer_loading logic

Test plan, ktd:
1) Search "henry" on the top search bar, notice 1 result is auto suggested
2) Ignore the auto-complete, press 'Enter' instead
3) Notice no results are returned.
4) Apply patch. Repeat.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 6 Katrin Fischer 2024-03-12 21:08:23 UTC
(In reply to Pedro Amorim from comment #4)
> I believe this defer_loading check was added here to have the "back" browser
> behavior work in the event that a search only returned 1 result and the
> cgi->redirect to that result kicks in (as opposed to showing the search
> results with just 1 entry).
> Is this cgi->redirect (in members/member.pl) still needed now that we have
> auto-complete suggestions?

I believe we did have the auto-suggest before this was added, it's a bit of a different use case. Having the back button work would be preferrable, but maybe we can fix the immediate issue first and further improve again in another bug.
Comment 7 Brendan Lawlor 2024-03-12 21:22:40 UTC
Not sure if I'm missing something, but with this patch applied the back button still works how I would expect it to.

If I search from the main page for Henry, it takes me directly to that patron, and the back button takes me back to the main page.

If I search for Bob it takes me to the results page with two entries. Then if I search for Henry it takes me to the patron record and back takes me back to the results page.
Comment 8 Jonathan Druart 2024-03-13 05:46:54 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2024-03-13 05:47:36 UTC
defer_loading is coming from 

  commit f6394501f2ae6468cef8279ba8e296427cb69188
  Bug 30063: Restore back behaviour

See 30063 comment 23 (point 6).

To recreate:
1. search 'henry'
2. the table loads, only one result
3. redirected to the patron detail view
4. go back
5. the table loads, only one result
6. redirected to the patron detail view

5 should not display the table result.

It's the behaviour on master, which is broken by this patch.


The regression has been introduced by
  commit f3bd0e7ebddac73552dd175b39d458855d7d663e
  Bug 34478: Manual fix - change to get remove csrf token - member-search-box
where we changed from POST to GET and so we do no longer set the variable:

members/member.pl:72 my $defer_loading = $input->request_method() eq "GET"  && !$circsearch ? 1 : 0;
Comment 10 Jonathan Druart 2024-03-13 09:32:52 UTC
Another regression/behaviour change is that hitting http://localhost:8081/cgi-bin/koha/members/members-home.pl will load the table and display the X first patrons.
Comment 11 Pedro Amorim 2024-03-13 09:34:48 UTC
Having discussed this with Jonathan, the regression he mentions on
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36302#c9

is only reproduce able with Firefox, not Chrome.
Comment 12 Pedro Amorim 2024-03-13 10:32:02 UTC
Created attachment 163105 [details] [review]
Bug 36302: Use replace instead of href

This will change URL but not fiddle with browser history.
Works the same (and as expected) in both Chrome and Firefox
Comment 13 Pedro Amorim 2024-03-13 10:32:04 UTC
Created attachment 163106 [details] [review]
Bug 36302: Remove unused code

Please can someone tell me why we need this code?
Inititally I thought this is where the redirect was coming from, but it's coming from JS, not here, so why do we need this?
Comment 14 Jonathan Druart 2024-03-13 10:53:49 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2024-03-13 10:54:24 UTC
(In reply to Pedro Amorim from comment #12)
> Created attachment 163105 [details] [review] [review]
> Bug 36302: Use replace instead of href
> 
> This will change URL but not fiddle with browser history.
> Works the same (and as expected) in both Chrome and Firefox

This is not fixing comment 10.
Comment 16 Jonathan Druart 2024-03-13 10:56:25 UTC
(In reply to Pedro Amorim from comment #13)
> Created attachment 163106 [details] [review] [review] [review]
> Bug 36302: Remove unused code
> 
> Please can someone tell me why we need this code?
> Inititally I thought this is where the redirect was coming from, but it's
> coming from JS, not here, so why do we need this?

We need it.
If you search for 42 you want the patron with cardnumber=42 to be displayed, not the table with all the occurrences of "42".
Comment 17 Pedro Amorim 2024-03-13 11:25:26 UTC
Created attachment 163108 [details] [review]
Bug 36302: Get rid of defer_loading

Update JS submit of patron_search_form to check for URLParams instead of defer_loading.
This is to keep behavior of not doing a search when accessing http://localhost:8081/cgi-bin/koha/members/members-home.pl directly
Comment 18 Pedro Amorim 2024-03-13 11:25:29 UTC
Created attachment 163109 [details] [review]
Bug 36302: use replace instead of href

This will change URL but not fiddle with browser history.
Works the same (and as expected) in both Chrome and Firefox

Test plan, ktd:
1) Search henry on the top search bar, notice 1 result is auto suggested
2) Ignore the auto-complete, press 'Enter' instead
3) Notice no results are returned.
4) Apply patch. Repeat.
5) Notice you get redirected to the single result of "henry"
6) Press the browser 'back' button and notice it works as expected (both Chrome+Firefox)
7) Visit patrons home directly:
http://localhost:8081/cgi-bin/koha/members/members-home.pl
8) Notice no search is performed
Comment 19 Pedro Amorim 2024-03-13 12:21:03 UTC
(In reply to Brendan Lawlor from comment #7)
> Not sure if I'm missing something, but with this patch applied the back
> button still works how I would expect it to.
> 
> If I search from the main page for Henry, it takes me directly to that
> patron, and the back button takes me back to the main page.
> 
> If I search for Bob it takes me to the results page with two entries. Then
> if I search for Henry it takes me to the patron record and back takes me
> back to the results page.

Thank you Brendan for your initial sign-off. I've since followed up while discussing with Jonathan's in case you want to take a look again,
Comment 20 Jonathan Druart 2024-03-13 12:27:48 UTC
Still not happy with it, under Firefox:
* Go to mainpage
* Click "Patrons" in the header
=> You are on http://localhost:8081/cgi-bin/koha/members/members-home.pl
* "Search for patrons": Search: "henry" (form on the left)
=> Redirected to the patron detail page
* back
=> You are now on the mainpage! We expect the patron search page.
Comment 21 Pedro Amorim 2024-03-13 15:41:53 UTC
Created attachment 163113 [details] [review]
Bug 36302: Revert back to POST

Pass the csrf token and 'cud-noop'

After having discussed with Joubu and multiple failed attempts at fixing the issue AND restoring previous behavior, this is the fix we agreed on.

This will change URL but not fiddle with browser history.
Works the same (and as expected) in both Chrome and Firefox

Test plan, ktd:
1) Search henry on the top search bar, notice 1 result is auto suggested
2) Ignore the auto-complete, press 'Enter' instead
3) Notice no results are returned.
4) Apply patch. Repeat.
5) Notice you get redirected to the single result of henry
6) Press the browser 'back' button and notice it works as expected (both Chrome+Firefox)
7) Visit patrons home directly:
http://localhost:8081/cgi-bin/koha/members/members-home.pl
8) Notice no search is performed
Comment 22 Lucas Gass (lukeg) 2024-03-13 21:29:39 UTC
Created attachment 163121 [details] [review]
Bug 36302: Revert back to POST

Pass the csrf token and 'cud-noop'

After having discussed with Joubu and multiple failed attempts at fixing the issue AND restoring previous behavior, this is the fix we agreed on.

This will change URL but not fiddle with browser history.
Works the same (and as expected) in both Chrome and Firefox

Test plan, ktd:
1) Search henry on the top search bar, notice 1 result is auto suggested
2) Ignore the auto-complete, press 'Enter' instead
3) Notice no results are returned.
4) Apply patch. Repeat.
5) Notice you get redirected to the single result of henry
6) Press the browser 'back' button and notice it works as expected (both Chrome+Firefox)
7) Visit patrons home directly:
http://localhost:8081/cgi-bin/koha/members/members-home.pl
8) Notice no search is performed

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 23 David Nind 2024-03-13 21:45:36 UTC
Created attachment 163122 [details] [review]
Bug 36302: Revert back to POST

Pass the csrf token and 'cud-noop'

After having discussed with Joubu and multiple failed attempts at fixing the issue AND restoring previous behavior, this is the fix we agreed on.

This will change URL but not fiddle with browser history.
Works the same (and as expected) in both Chrome and Firefox

Test plan, ktd:
1) Search henry on the top search bar, notice 1 result is auto suggested
2) Ignore the auto-complete, press 'Enter' instead
3) Notice no results are returned.
4) Apply patch. Repeat.
5) Notice you get redirected to the single result of henry
6) Press the browser 'back' button and notice it works as expected (both Chrome+Firefox)
7) Visit patrons home directly:
http://localhost:8081/cgi-bin/koha/members/members-home.pl
8) Notice no search is performed

Signed-off-by: David Nind <david@davidnind.com>
Comment 24 Jonathan Druart 2024-03-14 08:18:41 UTC
I am sorry but... I am still getting the redirect to the patron detail page after hitting the back button.

BUT I have tried v23.11.00 and it behaving like that as well.

Sure I tried yesterday a pre-csrf and it works "working" (no redirect).

David, what did you notice during your testing session?
Comment 25 Nick Clemens (kidclamp) 2024-03-14 14:59:15 UTC
(In reply to Jonathan Druart from comment #24)
> I am sorry but... I am still getting the redirect to the patron detail page
> after hitting the back button.
> 
> BUT I have tried v23.11.00 and it behaving like that as well.
> 
> Sure I tried yesterday a pre-csrf and it works "working" (no redirect).
> 
> David, what did you notice during your testing session?

I tested this a bit today, there are slightly different results when searching from mainpage vs members-home - the former using patron-search-box the latter member-search-box. Some of this is covered by discussion, just highlighting it all together

From mainpage the url is:
http://localhost:8081/cgi-bin/koha/members/member.pl?quicksearch=1&circsearch=1&searchmember=henry

From members-home the url is:
http://localhost:8081/cgi-bin/koha/members/member.pl?quicksearch=1&searchmember=henry&searchfieldstype=standard&searchtype=starts_with&branchcode_filter=&categorycode_filter=

circsearch makes the difference in that without that, the table is blank, this seems to be from defer_loading which prevents the form submission

Chrome and Firefox behave differently because it seems Chrome/Edge is designed to prevent a redirect when clicking back - Firefox does not do this

When the patch changes from GET to POST this effects the defer_loading variable so really seems to be effectively the same as the initial patch. 

My only thought has been to store the initial page or pass it through as a variable, and offer a "Back to {page}" link on the patron screen
Comment 26 Jonathan Druart 2024-03-14 15:58:06 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2024-03-14 15:59:16 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2024-03-14 16:01:10 UTC
Nick, do you know what is the behaviour in stable branches?
Comment 29 Nick Clemens (kidclamp) 2024-03-14 16:07:03 UTC
(In reply to Jonathan Druart from comment #28)
> Nick, do you know what is the behaviour in stable branches?
On a 23.05 site

In firefox I get redirected to patron if I hit back

In chrome I get the member search with no results when I hit back after searching for a patron from the members module
If I perform a checkout search from anywhere else, back takes me to the original page, i.e. Serials home
Comment 30 Julian Maurice 2024-03-15 10:48:46 UTC
From the test plan:
> 6) Press the browser 'back' button and notice it works as expected (both Chrome+Firefox)
What is the expected behaviour ? I get "this page has expired" error
Comment 31 Pedro Amorim 2024-03-15 11:18:03 UTC
I don't agree with this behavior, i.e. redirecting to the single search result if only one is returned:
1) Maybe this is the norm? But I have personally not seen other search systems with this behavior.
2) We're not consistent: Some tables in Koha (most?) return a single result if only one is found, not redirect happens.
3) It's artificial, unexpected and a pain to maintain, as we've proven here.

Having said that, we've gone to great lengths in making sure this historic behavior is kept, and it appears we've reached a consensus that, in Firefox, when clicking the "back" button from the patron detail page, being redirected to it again is what was happening before the CSRF work, meaning this patch fixes the search issue introduced by CSRF and keeps the original behaviour.

(In reply to Julian Maurice from comment #30)
> From the test plan:
> > 6) Press the browser 'back' button and notice it works as expected (both Chrome+Firefox)
> What is the expected behaviour ? I get "this page has expired" error

When I first tested my patch I was sure it had the same behavior on both browsers, but having tested it again after Joubu's comment, it appears it does not.

The current expected behavior:
Chrome: You get redirected to whatever page you started the header patron search from. 
Firefox: You get redirected to patrons-home and immediately redirected to the single patron detail page afterwards, regardless of where you started the header patron search form.
Comment 32 Pedro Amorim 2024-03-15 11:41:01 UTC
Created attachment 163207 [details] [review]
Bug 36302: [ALTERNATIVE] Get rid of defer_loading

Same test plan as the other patch, behavior is expected to be the same on both

But this alternative removes technical debt, whereas the other patch adds.
Comment 33 Pedro Amorim 2024-03-15 11:46:05 UTC
The noop approach is/was hacky and it doesn't seem to fix the issue entirely anyway.
I'm submitting my first approach again as an alternative patch and let others decide which route is better.
There may of course be a better approach at fixing this that I've not been able to come up with, open to input as always.
Comment 34 Katrin Fischer 2024-03-15 12:00:59 UTC
(In reply to Pedro Amorim from comment #31)
> I don't agree with this behavior, i.e. redirecting to the single search
> result if only one is returned:
> 1) Maybe this is the norm? But I have personally not seen other search
> systems with this behavior.

I think this is/was about saving clicks for the circulation desk mostly. If you scan a card, they want to go directly to the patron account. I am not sure if it's also applied for other searches/results in general. But I believe it's always been the case for cardnumber searches (since 3.2 at least, when I began using Koha).

I believe if you picked an entry, using the auto-complete, you'd also want/expect this behavior.

(Not sure those comments fully apply here, just trying to give a user point of view)

> 2) We're not consistent: Some tables in Koha (most?) return a single result
> if only one is found, not redirect happens.
> 3) It's artificial, unexpected and a pain to maintain, as we've proven here.
> 
> Having said that, we've gone to great lengths in making sure this historic
> behavior is kept, and it appears we've reached a consensus that, in Firefox,
> when clicking the "back" button from the patron detail page, being
> redirected to it again is what was happening before the CSRF work, meaning
> this patch fixes the search issue introduced by CSRF and keeps the original
> behaviour.
Comment 35 Nick Clemens (kidclamp) 2024-03-15 12:20:13 UTC
(In reply to Katrin Fischer from comment #34)
> (In reply to Pedro Amorim from comment #31)
> > I don't agree with this behavior, i.e. redirecting to the single search
> > result if only one is returned:
> > 1) Maybe this is the norm? But I have personally not seen other search
> > systems with this behavior.
> 
> I think this is/was about saving clicks for the circulation desk mostly. If
> you scan a card, they want to go directly to the patron account. 

Cardnumber searches are still handled directly with a redirect in the script

The case here is a standard search that returns a single patron
Comment 36 Katrin Fischer 2024-03-15 12:23:21 UTC
(In reply to Nick Clemens from comment #35)
> (In reply to Katrin Fischer from comment #34)
> > (In reply to Pedro Amorim from comment #31)
> > > I don't agree with this behavior, i.e. redirecting to the single search
> > > result if only one is returned:
> > > 1) Maybe this is the norm? But I have personally not seen other search
> > > systems with this behavior.
> > 
> > I think this is/was about saving clicks for the circulation desk mostly. If
> > you scan a card, they want to go directly to the patron account. 
> 
> Cardnumber searches are still handled directly with a redirect in the script
> 
> The case here is a standard search that returns a single patron

Hi Nick, thanks for clarifying! For another search I'd day it's not that important (but no guarantee everyone agrees with me) :)
Comment 37 Nick Clemens (kidclamp) 2024-03-15 12:26:56 UTC
Created attachment 163226 [details] [review]
Bug 36302: [ALTERNATIVE] Get rid of defer_loading

Same test plan as the other patch, behavior is expected to be the same on both

But this alternative removes technical debt, whereas the other patch adds.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 38 Pedro Amorim 2024-03-15 12:30:29 UTC
(In reply to Katrin Fischer from comment #34)
> I think this is/was about saving clicks for the circulation desk mostly. If
> you scan a card, they want to go directly to the patron account. I am not
> sure if it's also applied for other searches/results in general. But I
> believe it's always been the case for cardnumber searches (since 3.2 at
> least, when I began using Koha).
> 
> I believe if you picked an entry, using the auto-complete, you'd also
> want/expect this behavior.
> 
> (Not sure those comments fully apply here, just trying to give a user point
> of view)

The following use case:
"If you scan a card, they want to go directly to the patron account."
This is already handled in member.pl. i.e. this is already ensured, regardless of the patches in this bug or the issue described by it. This functionality is not broken and is still in place.

This:
"I believe if you picked an entry, using the auto-complete, you'd also want/expect this behavior."
I agree, and this is also already the case as well. Again, regardless of the patches in this bug or the issue described by it. When you click on any of the suggestions given by the auto-complete, you're forwarded directly to the patron you clicked on.

The behavior that I don't agree with:
"Search -> Browser goes to search results page -> JavaScript checks if only 1 result found -> Browser redirects user to this single result detail page"
Doesn't happen in either of the use-cases above.

A search is a search, if you submit a search then you should be presented with a search results page, regardless of how many results are found.
Having a special case for: if a cardnumber is submitted, which is a unique identifier, Koha forwards you directly to the patron detail page and it does NOT first visit the search results page -> This is fine
If you click on an auto-complete suggestion, Koha forwards you to the patron detail page and it does NOT first visit the search results page -> This is fine (and expected).
Comment 39 Jonathan Druart 2024-03-15 12:42:37 UTC
I've tested the ALTERNATIVE patch alone and I get a weird JS error in the console:

Uncaught SyntaxError: unexpected token: '!'

http://localhost:8081/cgi-bin/koha/members/members-home.pl
Comment 40 Pedro Amorim 2024-03-15 12:57:57 UTC
(In reply to Jonathan Druart from comment #39)
> I've tested the ALTERNATIVE patch alone and I get a weird JS error in the
> console:
> 
> Uncaught SyntaxError: unexpected token: '!'
> 
> http://localhost:8081/cgi-bin/koha/members/members-home.pl

I've retried the ALTERNATIVE patch alone again on current master on both Chrome and Firefox, I'm unable to reproduce this.
Comment 41 Jonathan Druart 2024-03-15 13:13:24 UTC
(In reply to Pedro Amorim from comment #40)
> (In reply to Jonathan Druart from comment #39)
> > I've tested the ALTERNATIVE patch alone and I get a weird JS error in the
> > console:
> > 
> > Uncaught SyntaxError: unexpected token: '!'
> > 
> > http://localhost:8081/cgi-bin/koha/members/members-home.pl
> 
> I've retried the ALTERNATIVE patch alone again on current master on both
> Chrome and Firefox, I'm unable to reproduce this.

Hum, retried and the error disappeared after a reset_all + restart of Firefox. Weird because I had the error with the patch applied and not on master. Forget that for now, I will keep an eye on it in case it pops up again.
Comment 42 Jonathan Druart 2024-03-20 05:04:33 UTC
Created attachment 163487 [details] [review]
Bug 36302: Get rid of defer_loading in patron-search

Same test plan as the other patch, behavior is expected to be the same on both

But this alternative removes technical debt, whereas the other patch adds.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 43 Jonathan Druart 2024-03-20 05:05:13 UTC
Not convinced this is the best we can do but at least we are fixing the bad bug. We will see for the side-effects later.
Comment 44 Katrin Fischer 2024-03-20 17:40:29 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 45 Fridolin Somers 2024-03-21 13:55:34 UTC
Is this needed for backport ?
I applyed on 23.11.x and the behavior is :
1) Enter "henry" on the top search bar
2) Press 'Enter'
=> There is "henry" in left form but the form is not submitted anymore!
Comment 46 Martin Renvoize (ashimema) 2024-03-22 06:39:01 UTC
It's a fix for a bug caused by the csrf work.. so I don't believe it's required for your branch fridolin.
Comment 47 Fridolin Somers 2024-03-22 08:17:31 UTC
OK thanks
No backport
Comment 48 Jonathan Druart 2024-03-25 12:21:48 UTC
Regression: patron list is loaded when placing a hold:

http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=9
Comment 49 Pedro Amorim 2024-03-26 12:30:59 UTC
Created attachment 163917 [details] [review]
Bug 36302: Reinstate defer_loading

defer_loading is now a patron_search_js BLOCK param - default on
Comment 50 Pedro Amorim 2024-03-26 12:31:02 UTC
Created attachment 163918 [details] [review]
Bug 36302: members-home.pl defers loading, member.pl does not
Comment 51 Katrin Fischer 2024-03-26 14:42:14 UTC
Can we have QA here please? :)
Comment 52 Jonathan Druart 2024-03-27 07:33:38 UTC
I am wondering if loading the table is expected when coming from this link:

koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
233                                     <a class="btn btn-link" href="/cgi-bin/koha/members/member.pl" class="toggler save_entryform">

We will fix later if needed.
Comment 53 Jonathan Druart 2024-03-27 07:34:27 UTC
Created attachment 163974 [details] [review]
bug 36302: Restore passing deferLoading to DT
Comment 54 Jonathan Druart 2024-03-27 07:34:56 UTC
You have not put back defer_loading to DT's options, was it expected? If we don't the GET is done (ie. we will fetch the first 20 patrons).
Comment 55 Jonathan Druart 2024-03-27 07:35:55 UTC
+        let defer_loading = Number( '[% defer_loading | html %]' || 1 );

This is kind of working but it reads really bad IMO.

Number ('0' || 1) => 0
Number (0 || 1) => 1

Javascript...
Comment 56 Pedro Amorim 2024-03-27 09:19:29 UTC
(In reply to Jonathan Druart from comment #54)
> You have not put back defer_loading to DT's options, was it expected? If we
> don't the GET is done (ie. we will fetch the first 20 patrons).

Thanks Jonathan, I left it out because I couldn't tell what it was doing (saw no difference with or without) and according to docs is deprecated:
https://datatables.net/reference/option/deferLoading

It was intentional, but I don't feel strongly about it.
Comment 57 Jonathan Druart 2024-03-27 12:43:11 UTC
(In reply to Pedro Amorim from comment #56)
> (In reply to Jonathan Druart from comment #54)
> > You have not put back defer_loading to DT's options, was it expected? If we
> > don't the GET is done (ie. we will fetch the first 20 patrons).
> 
> Thanks Jonathan, I left it out because I couldn't tell what it was doing
> (saw no difference with or without) and according to docs is deprecated:
> https://datatables.net/reference/option/deferLoading
> 
> It was intentional, but I don't feel strongly about it.

Either we add it back or we need to workaround again... You decide :D
You should avoid the unnecessary GET. Especially because you will see the full table before the actual fitlering (it's blinking: table shows with full results then it's updated with the results of the user's request).
Comment 58 Pedro Amorim 2024-03-27 13:42:49 UTC
Created attachment 164002 [details] [review]
bug 36302: Restore passing deferLoading to DT

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 59 Pedro Amorim 2024-03-27 13:43:51 UTC
I couldn't see the blinking but there was definitely a GET happening in the background. I think this should be good to go as is?
Comment 60 Jonathan Druart 2024-03-27 13:59:56 UTC
Created attachment 164003 [details] [review]
Bug 36302: Reinstate defer_loading

defer_loading is now a patron_search_js BLOCK param - default on

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 61 Jonathan Druart 2024-03-27 13:59:59 UTC
Created attachment 164004 [details] [review]
Bug 36302: members-home.pl defers loading, member.pl does not

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 62 Jonathan Druart 2024-03-27 14:00:02 UTC
Created attachment 164005 [details] [review]
bug 36302: Restore passing deferLoading to DT

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 63 Katrin Fischer 2024-04-02 15:10:29 UTC
Follow-up "Restore passing deferLoading to DT" pushed to master.
Comment 64 Katrin Fischer 2024-04-02 16:00:55 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 65 Lucas Gass (lukeg) 2024-04-02 21:24:42 UTC
(In reply to Katrin Fischer from comment #64)
> Pushed for 24.05!
> 
> Well done everyone, thank you!

I don't think all the patches were pushed. 

Only bug 36302: Restore passing deferLoading to DT.
Comment 66 Katrin Fischer 2024-04-02 21:27:23 UTC
Note to myself: push second and third patch too!
Comment 67 Katrin Fischer 2024-04-03 07:54:00 UTC
Missing patches pushed to master!
Comment 68 Emmi Takkinen 2024-04-03 11:51:22 UTC
I'm currently facing problem with patron search in current master. There is an error in browsers console
> Uncaught ReferenceError: defer_loading is not defined

It seems last patch here is actually missing the variable defer_loading.
Comment 69 Jonathan Druart 2024-04-03 12:36:42 UTC
(In reply to Emmi Takkinen from comment #68)
> I'm currently facing problem with patron search in current master. There is
> an error in browsers console
> > Uncaught ReferenceError: defer_loading is not defined
> 
> It seems last patch here is actually missing the variable defer_loading.

I do not see that. How to recreate the error?
Comment 70 Pedro Amorim 2024-04-03 12:39:59 UTC
(In reply to Emmi Takkinen from comment #68)
> I'm currently facing problem with patron search in current master. There is
> an error in browsers console
> > Uncaught ReferenceError: defer_loading is not defined
> 
> It seems last patch here is actually missing the variable defer_loading.

defer_loading was reinstated by patch "Reinstate defer_loading".
Current master now contains all 4 patches, but for a while it only contained the first and last patch here (out of 4 total).
Can you `git pull master` and try again? I've tested and cannot reproduce this.
Comment 71 Emmi Takkinen 2024-04-03 12:45:24 UTC
(In reply to Jonathan Druart from comment #69)
> (In reply to Emmi Takkinen from comment #68)
> > I'm currently facing problem with patron search in current master. There is
> > an error in browsers console
> > > Uncaught ReferenceError: defer_loading is not defined
> > 
> > It seems last patch here is actually missing the variable defer_loading.
> 
> I do not see that. How to recreate the error?

I updated my master just now again and error is gone and patron search is working. The problem seems to be that my master (updated it this morning) didn't have all patches from this patch (?) so defer_loading was missing. So all good here, sorry!
Comment 72 Katrin Fischer 2024-04-18 21:24:40 UTC
Hi Frido, why add the keyword, when this is already pushed?
Comment 73 Fridolin Somers 2024-04-22 05:32:28 UTC
(In reply to Katrin Fischer from comment #72)
> Hi Frido, why add the keyword, when this is already pushed?

Sorry it was to remember no backport to 23.11.x ;)
Comment 74 Fridolin Somers 2024-05-22 12:23:59 UTC
Looks resolved